Repository navigation
Different behavior for type reference directive in nodenext based on whether types are in @types or not聽#47444
Description
Bug Report
馃攷 Search Terms
type reference directive node12 nodenext
馃捇 Fourslash server test case
I used fourslash server tests to verify this because it treats files in node_modules as a real tsc run would, whereas the compiler test suite treats them as if they鈥檙e root files for compilation, which messes up what I鈥檓 trying to demonstrate.
I have two dependencies in node_modules here that are structurally identical, but one is inside @types and the other is not. The one in @types is resolved by the primaryLookup of resolveTypeReferenceDirective in moduleNameResolver.ts, which
- only looks in
typeRoots(including@types) - only accepts
.tsand.d.tsfiles (not the cts/mts variants) - does not look at
exports
The one outside of @types, by merit of not being in typeRoots, uses the secondaryLookup function, which uses a very different algorithm that brings in some Node12/NodeNext resolution features. It
- looks in
node_modules/*first before tryingnode_modules/@types/* - accepts cts/mts file extensions
- looks at
exportswith conditionsnode, require, types
This test case shows how this discrepancy can make a difference in resolution based only on whether the package is inside typeRoots or not.
I don鈥檛 know what the expected behavior is here, but it feels like it should probably be the same whether the package is in @types or regular node_modules.
// @Filename: /tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "nodenext",
//// "types": ["inside-at-types", "outside-at-types"]
//// }
//// }
// @Filename: /node_modules/@types/inside-at-types/package.json
//// {
//// "name": "@types/inside-at-types",
//// "version": "1.0.0",
//// "types": "./index.d.ts",
//// "exports": {
//// ".": {
//// "default": "./main.mjs"
//// }
//// }
//// }
// @Filename: /node_modules/@types/inside-at-types/index.d.ts
//// export {};
//// declare global {
//// var typesFieldInsideAtTypes: any;
//// }
// @Filename: /node_modules/@types/inside-at-types/main.d.mts
//// export {};
//// declare global {
//// var exportsInsideAtTypes: any;
//// }
// @Filename: /node_modules/outside-at-types/package.json
//// {
//// "name": "outside-at-types",
//// "version": "1.0.0",
//// "types": "./index.d.ts",
//// "exports": {
//// ".": {
//// "default": "./main.mjs"
//// }
//// }
//// }
// @Filename: /node_modules/outside-at-types/index.d.ts
//// export {};
//// declare global {
//// var typesFieldOutsideAtTypes: any;
//// }
// @Filename: /node_modules/outside-at-types/main.d.mts
//// export {};
//// declare global {
//// var exportsOutsideAtTypes: any;
//// }
// @Filename: /index.ts
//// // One pair of these should be resolved;
//// // the other unresolved. Currently, they're mixed.
////
//// typesFieldInsideAtTypes;
//// typesFieldOutsideAtTypes;
//// exportsInsideAtTypes;
//// exportsOutsideAtTypes;
goTo.file("/index.ts");
verify.baselineSyntacticAndSemanticDiagnostics();馃檨 Actual behavior
exportsInsideAtTypes and typesFieldOutsideAtTypes are unresolved (meaning inside @types resolves to the types field while outside @types resolves to the exports field)
馃檪 Expected behavior
Either both typesFieldInsideAtTypes and typesFieldOutsideAtTypes or both exportsInsideAtTypes and exportsOutsideAtTypes should be unresolved, while the other pair is resolved.
(Daniel Rosenwasser (@DanielRosenwasser) Wesley Wigham (@weswigham))
Activity
andrewbranch commentedon Jan 14, 2022
Note: there are quite possibly differences between
primaryLookupandsecondaryLookupobservable without node12/nodenext, butexportsmakes it really easy to find a repro.weswigham commentedon Jan 14, 2022
I agree that they should definitely behave the same - I image we probably need to pass forward the settings used for
primaryLookuptosecondaryLookup. (Which means we shouldn't respect exports for triple slash references). Once that's consistent, we can discuss weather triple slash references should use non-old-cjs resolution settings under some circumstances.andrewbranch commentedon Aug 18, 2022
Wesley Wigham (@weswigham) I鈥檓 coming back around to this, but I feel like your assessment above may have changed in light of #47732. I feel like it would make sense to base the resolution mode of automatic type reference directives on the package.json
type. That would make automatic type references behave the same as manual type references inside a.tsroot file. Thoughts?andrewbranch commentedon Aug 18, 2022
Further, it seems odd to me that triple-slash references, regardless of requesting resolution mode, wouldn鈥檛 use
exportswhen both CJS- and ESM-mode imports do.You might say that a triple-slash reference isn鈥檛 an import, so it doesn鈥檛 seem obvious that it should care about exports. But triple-slash references have always pulled from the
typesfield which is analogous to themainfield, which is the place where arequireof the package name resolves to. So extending that analogy, it seems odd not to look forexports["."]in a mode where arequire/importwould look there. Especially considering the possible existence of ESM-only dependencies in an ESM-only project, where the dependency might not feel the need to specify amainortypesat all.weswigham commentedon Aug 18, 2022
Yeah, the important note was consistency in resolution between the two scopes - it's certainly more likely useful to always follow exports/other node style resolution rules for triple slash refs. (Though it'd probably annoy some people who've repurposed them over at deno :) ) It should definitely remain independent of the project's
moduleResolution, I think.andrewbranch commentedon Aug 18, 2022
Wait, why should it be independent of
moduleResolution? It鈥檚 not independent ofmoduleResolutiontoday because the secondary lookup does use themoduleResolutionState.featuresbased on module resolution.weswigham commentedon Aug 18, 2022
I'm thinking we should probably have triple-slash refs always use, eg,
nodenextrules, this way refs work across DT regardless of the consuming package's tsconfig settings/if they are inside a typeRoot.Also, the secondary lookup does use the features passed in - but type reference resolution already always passes in a fixed feature set, regardless of compiler options.
andrewbranch commentedon Aug 18, 2022
I don鈥檛 think so
TypeScript/src/compiler/moduleNameResolver.ts
Line 359 in 1592210
weswigham commentedon Aug 18, 2022
Riiiight, I changed it from always-cjs to option-dependent when I added the mode flags... It should probably be changed to always-latest-node (which I mentioned as an option in that code comment near where you linked). The further proliferation of resolution settings makes me seriously want to avoid having a lot of variants of how TS resolves triple-slash refs just because we're forced to have a lot of variants for imports. Since only we read them, we can choose just one standard resolution scheme - it doesn't need to be configurable.
andrewbranch commentedon Aug 18, 2022
I think that seems reasonable, but do you still want to allow CJS/ESM mode overrides? If you want to do
nodenext-ish resolution for type reference directives no matter themoduleResolutionsetting, we will need to pick a set ofconditionsand whetherNodeResolutionFeatures.EsmModeis set in modes where those are otherwise meaningless.weswigham commentedon Aug 18, 2022
Yeah, I think the mode override is meaningful for pulling in augmenting modules only accessible under import/require conditions, and the conditions should probably be the same
node, types, import || requirelist. The janky thing, I guess, is the the containing file's mode determines the default mode of the reference, as it does now and with imports. Even if themoduleResolutiondoesn't imply the containing file should have a mode. We'd have to change file mode calculation to be unconditional (even if we don't use it during emit or normal import module resolution), rather than only doing it whenmoduleResolutionis a nodeish one, to get that to work.andrewbranch commentedon Aug 18, 2022
For the proposed
minimalandhybrid, I鈥檓 fairly comfortable with saying it鈥檚 alwaysimport. Fornode, 馃し?weswigham commentedon Aug 18, 2022
I don't think the condition TS uses should depend on the resolution mode of the project - it'd make using the same declaration file in a minimal, node, and node16 project pretty hard. Given that, always doing the nodenext mode calculation seems like the way to go, insofar as it probably minimizes the need for mode assertions.
andrewbranch commentedon Aug 18, 2022
But in those other modes, package.json
typeexplicitly doesn鈥檛 matter, nor do.ctsand.mtsextensions. I think I don鈥檛 even want to setimpliedNodeFormatin them. We could set bothimportandrequire馃槄. I would suggest neither, but I think that would break things.