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

Support resolution-mode overrides in type-only constructs in all moduleResolution modes #55725

Merged
merged 10 commits into from
Sep 26, 2023

Conversation

andrewbranch
Copy link
Member

This PR + the passage of time fixes #55579

Affects the following syntaxes:

  • triple-slash references: <reference types="foo" resolution-mode="require" />
  • import() types: type Foo = typeof import("foo", { assert: { "resolution-mode": "import" } })
  • type-only imports: import type foo from "foo" assert { "resolution-mode": "require" }

(The latter two are still errors outside of nightly due to the use of import assertions, but these examples will be replaceable with import attributes after #54242.) The semantics are changed as follows in moduleResolution modes:

  • node16/nodenext: no changes
  • bundler and node10:
    • no longer errors
    • the presence of an override enables imports/exports resolution even if disabled for normal imports via node10 or resolvePackageJsonImports/resolvePackageJsonExports, since these are intended to signal information about the runtime module resolver, and a resolution-mode override is a TypeScript-only construct
    • the presence of an override affects matching conditions for imports/exports resolution
  • classic:
    • no longer errors

Notes:

  • There’s a tension between wanting all TS-only resolutions to behave similarly to each other across moduleResolution modes and wanting those resolutions to behave similarly to their non-TS-only resolutions in the same moduleResolution mode. While it sounds reasonable to propose “triple-slash references should resolve the same in every moduleResolution mode,” it gets uglier when thinking about type-only imports:

    import type {} from "./foo" assert { "resolution-mode": "import" }

    This is an error in nodenext since ESM-mode resolutions require file extensions ("./foo.js"). It makes sense for this resolution to behave how a runtime import would. But in --moduleResolution bundler, file extensions are never required, so by the same logic, this import should work under that mode. So, I made the choice to vary the resolution algorithm for each of the supporting syntaxes only by forcing support for imports/exports and setting the appropriate conditions—in other words, "resolution-mode": "import" doesn’t imply Node.js ESM resolution outside of node16/nodenext.

  • classic doesn’t resolve node_modules in the first place (except for @types) so it doesn’t seem very compelling to try to add support for resolution-mode assertions there, so I opted to just remove the error and ignore them.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 12, 2023
break;
case ModuleResolutionKind.Classic:
result = classicNameResolver(moduleName, containingFile, compilerOptions, host, cache, redirectedReference);
break;
case ModuleResolutionKind.Bundler:
result = bundlerModuleNameResolver(moduleName, containingFile, compilerOptions, host, cache, redirectedReference);
result = bundlerModuleNameResolver(moduleName, containingFile, compilerOptions, host, cache, redirectedReference, resolutionMode);
Copy link
Member Author

Choose a reason for hiding this comment

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

In node16/nodenext, this resolutionMode is always set. In bundler and node10, it’s only set when the syntax has an explicit override.

It feels like it would be much nicer to have access to the syntax that triggered the request, or at least a serialization of all the import attributes, as part of this API. I can imagine other resolution-affecting options in import attributes in the future. But this change was possible without an API change. Open to doing something different though.

extensions: Extensions,
isConfigLookup: boolean,
redirectedReference: ResolvedProjectReference | undefined,
conditions?: readonly string[],
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this non optional in all the workers/Internal methods or signatures to ensure we are not ignoring any params anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

The exported functions with internal overloads already have many optional parameters in their public overload, so this won’t do anything there. Anywhere we call them with fewer parameters would just use the public overload. But I double checked them and they’re only called in tests.

src/compiler/moduleNameResolver.ts Show resolved Hide resolved
@@ -1411,13 +1430,13 @@ export function resolveModuleName(moduleName: string, containingFile: string, co
result = nodeNextModuleNameResolver(moduleName, containingFile, compilerOptions, host, cache, redirectedReference, resolutionMode);
break;
case ModuleResolutionKind.Node10:
result = nodeModuleNameResolver(moduleName, containingFile, compilerOptions, host, cache, redirectedReference);
result = nodeModuleNameResolver(moduleName, containingFile, compilerOptions, host, cache, redirectedReference, resolutionMode ? getConditions(compilerOptions, resolutionMode === ModuleKind.ESNext) : undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to consider pushing this conditions lookup out to the caller? Or is it too useful a shorthand to take a resolutionMode directly on resolveModuleName as meaning a lookup type override for node10/bundler? I'm just thinking that if we push it out to the caller, we can pass the conditions argument through to all resolvers (and not just the node10 and bundler ones), which'd be nice for API consumers of the resolver who are running funky custom import/import condition structures, but might be bad for our existing resolution cache APIs (namely, they'd have to go from being mode-aware to being condition-aware). It's probably a good compromise to leave it as-is, messing with the resolution cache's API is a lot of surface area to change just to make an edgecase API use nicer.

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 think it’s kind of confusing to push it out to the caller, particularly in the node10 case (which I guess we can care somewhat less about since it ought to be a deprecation candidate for 6.0) since that mode doesn’t do package.json imports/exports by default. While these APIs are internal anyway, I think for now I like only having resolutionMode exposed to the caller since that has a direct relationship with the "resolution-mode" attribute syntax.

@andrewbranch andrewbranch merged commit 1b70ac3 into main Sep 26, 2023
19 checks passed
@andrewbranch andrewbranch deleted the feature/55579 branch September 26, 2023 21:20
Ciornenichii

This comment was marked as spam.

let features = getNodeResolutionFeatures(options);
// Unlike `import` statements, whose mode-calculating APIs are all guaranteed to return `undefined` if we're in an un-mode-ed module resolution
// setting, type references will return their target mode regardless of options because of how the parser works, so we guard against the mode being

This comment was marked as spam.

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improve portability of resolution-mode assertions automatically emitted in nodenext
5 participants