Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for "typesVersions" redirects #26568

Merged
merged 14 commits into from
Sep 7, 2018
Merged

Adds support for "typesVersions" redirects #26568

merged 14 commits into from
Sep 7, 2018

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Aug 21, 2018

This adds support TypeScript version-specific redirects to type definitions in a package. If a package wishes to include type definitions that gracefully downgrade to older editions of TypeScript, they will be able to do so by adding the following entry to their package.json file:

{
  "name": "package-name",
  "version": "1.0",
  "types": "index",
  "typesVersions": {
    ">=3.1.0-0": { "*": ["ts3.1/*"] }
  }
}

When a "typesVersions" field is found in a referenced package, we search for the first version range that matches the current compiler version. If a match is found, the associated path patterns are applied.

Version ranges specified in "typesVersions" keys follow the same range comparator format (strict only) as https://github.com/npm/node-semver#ranges.

Path patterns are applied in the same way that the "paths" compiler option is applied to paths in your project.

This feature will only support compiler version of 3.1 or later. Earlier TypeScript compilers will use the existing behavior.

Fixes #22605

@rbuckton
Copy link
Member Author

There is some overlap between src/compiler/semver.ts and src/jsTyping/semver.ts. I may retire the version in jsTyping as its very specific to its implementation, while the implementation in compiler is more generic and can serve both cases. I can either address this in this PR or a follow up.

@weswigham
Copy link
Member

If a match is found, the value is inserted into the path between the package directory and "types" (or any named submodule beneath the package).

This feels strange. I'd expect either a module specifier (so it points directly at another package to defer to), or a direct path.

@rbuckton
Copy link
Member Author

This is more like a per-version root directory override. There are two kinds of typed packages we need to support:

  • Ambient module based packages (that generally have types defined in a single file like "index.d.ts")
  • Non-ambient module based packages (that use top level exports and use multiple files)

See https://gist.github.com/rbuckton/bcde1c2adde270c3b0d7ced5320c0877 for a more detailed description and examples.

@rbuckton
Copy link
Member Author

[…] I'd expect either a module specifier (so it points directly at another package to defer to), or a direct path.

Pointing to another package is not the intended scenario. This is intended to support the following two scenarios:

  1. Declarations defined via ambient modules in a single index.d.ts file (common on DefinitelyTyped)
  2. Declarations defined via one or more declaration files using top level exports (common on DefinitelyTyped and default behavior of TypeScript build outputs).

If it were a direct path, we could not support (2). We could consider some kind of path mapping/path template, but overall it would just add unnecessary complexity.

@rbuckton
Copy link
Member Author

I have a few open questions:

  • Should we ignore paths that traverse outside of the package (e.g. disallow absolute paths or "..")?
  • Should we compare against TypeScript's versionMajorMinor or version? While version seems to be more correct, due to the precedence rules of semver, 3.1 comes after 3.1.0-dev/3.1.0-next, so using a simple version number like 3.1 wouldn't work when running a dev/next/insiders build.
  • Should we consider a path templating feature, or leave it as a prefix path only?

@rbuckton
Copy link
Member Author

If we decide we want to support semver ranges in "typesVersions", I actually was able to put together a relatively lightweight semver range parser based on the BNF here: https://github.com/npm/node-semver#range-grammar

I put a copy of the version of semver.ts with support for ranges here in a gist so as not to pollute this PR with this change unless we decide to implement it.

@rbuckton rbuckton force-pushed the typesVersions branch 6 times, most recently from d350e7b to e02dd34 Compare August 22, 2018 18:38
@rbuckton
Copy link
Member Author

Should we consider a path templating feature, or leave it as a prefix path only?

If we want to leverage our existing path templating functionality, it would change "typesVersions" to look like this:

{
  "typesVersions": {
    "3.1": { "*": ["ts3.1/*"] }
  }
}

This would also require refactoring tryLoadModuleUsingBaseUrl to extract its logic and inject it in various places in moduleNameResolver.ts.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some declaration emit tests that write out paths (and maybe import quick fixes), ie.

// @declaration: true
// @filename: node_modules/foo/tsnew/index.d.ts
export interface Foo {}
export function foo(): Foo;
// @filename: node_modules/foo/tsnew/deep.d.ts
export interface Foo2 {}
export function foo2(): Foo2;
// @filename: node_modules/foo/index.d.ts
export interface Bar {}
export function bar(): Bar;
// @filename: node_modules/foo/deep.d.ts
export interface Bar2 {}
export function bar2(): Bar2;
// @filename: node_modules/foo/package.json
{
  "name": "foo",
  "version": 0.0.1,
  "types": "index.d.ts",
  "typesVersions": { "3.1": "tsnew/" }
}
// @filename: consumer.ts
import { foo } from "foo";
import { foo2 } from "foo/deep";
export const v = foo(); // will use an import type to name this in declaration emit
export const v2 = foo2();

Just because the moduleSpecifiers.ts file is separate from the resolver so I have low confidence that resolution changes are instantly reflected in declaration files.

@weswigham
Copy link
Member

Ah! Also a test of what we do when a package's "latest version" root file attempts to import the real root, eg

// @declaration: true
// @filename: node_modules/bar/tsnew/index.d.ts
export * from "../";
// @filename: node_modules/bar/tsnew/deep.d.ts
export * from "../deep";
// @filename: node_modules/bar/index.d.ts
export interface Bar {}
export function bar(): Bar;
// @filename: node_modules/bar/deep.d.ts
export interface Bar2 {}
export function bar2(): Bar2;
// @filename: node_modules/bar/package.json
{
  "name": "bar",
  "version": 0.0.1,
  "types": "index.d.ts",
  "typesVersions": { "3.1": "tsnew/" }
}
// @filename: consumer.ts
import { bar } from "bar";
import { bar2 } from "bar/deep";
export const v = bar();
export const v2 = bar2();

@weswigham
Copy link
Member

weswigham commented Aug 23, 2018

Also, I know I've said this in person, but do we know of any non-DT people we're writing this for? Because on DT this is all going to be managed by the gen script, so no human hand should ever touch these fields - meaning a pretty or simple configuration isn't required; a manifest of real paths to redirects would even be sufficient, eg

{
  "typesVersions": {
    "3.1": {
      "./index.d.ts": "./ts3.1/index.d.ts",
      "./deep.d.ts": "./ts3.1/deep.d.ts",
      "./sub/module/index.d.ts": "./ts3.1/sub/module/index.d.ts",
    }
  }
}

which avoids encoding the folder-only-redirects convention into the feature without going full path-mapping. If we know of some others who'd like to use this, it'd be great to get their input on what kind of configuration makes the most sense from the perspective of their build tools and source trees.

@ajafff
Copy link
Contributor

ajafff commented Aug 23, 2018

do we know of any non-DT people we're writing this for?

👋
I need this in tsutils to expose an API that is compatible with the TypeScript version you are compiling with.

Regarding submodule imports:
When importing from tsutils/util does it even look at tsutils/package.json? Node's module spec says it doesn't.

However, I want to redirect imports from package root as well as imports from any submodule:

  • tsutils -> tsutils/${tsVersion}
  • tsutils/typeguard/node -> tsutils/${tsVersion}/typeguard/node

@RyanCavanaugh
Copy link
Member

Also, I know I've said this in person, but do we know of any non-DT people we're writing this for?

Dojo and Angular have both voiced complaints about the inability for them to bundle typings of varying versions. They still have the problem of how to generate downlevel-compatible .d.ts files but that's sort of a separate issue.

@rbuckton
Copy link
Member Author

If we support path mappings, would we want completions to include both mapped and unmapped paths?

Consider the directory structure for node_modules/foo:

package.json
index.d.ts
aaa.d.ts
ts3.1/index.d.ts
ts3.1/zzz.d.ts

With the following in package.json:

{
  "typesVersions": {
    "3.1": { "*": ["ts3.1/*"] }
  }
}

If we follow the same logic for completions that we do for baseUrl/paths, then a completion for "foo/ would return index, aaa, ts3.1 (directory completion), and zzz (from path mapping).

Does that seem acceptable?

@rbuckton
Copy link
Member Author

@weswigham can you take another look?

@rbuckton rbuckton requested review from sandersn and a user August 30, 2018 18:28
if (!hasProperty(typesVersions, key)) continue;

const keyRange = VersionRange.tryParse(key);
if (keyRange === undefined) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case probably deserves a log message since it's basically a silent error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a verification pass to readPackageJsonTypesVersionPaths, since getPackageJsonTypesVersionsPaths is also used by services and doesn't take a ModuleResolutionState argument.

@cordiosd
Copy link

cordiosd commented Jun 15, 2019

#31907

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a back-compat mechanism for bundled .d.ts files in package.json
6 participants