Different behavior for type reference directive in nodenext based on whether types are in @types or not

[andrewbranch]

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’re root files for compilation, which messes up what I’m 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 .ts and .d.ts files (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 trying node_modules/@types/*
• accepts cts/mts file extensions
• looks at exports with conditions node, 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’t 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.

(@DanielRosenwasser @weswigham)

[andrewbranch]

Note: there are quite possibly differences between primaryLookup and secondaryLookup observable without node12/nodenext, but exports makes it really easy to find a repro.

[weswigham]

I agree that they should definitely behave the same - I image we probably need to pass forward the settings used for primaryLookup to secondaryLookup. (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]

@weswigham I’m 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 .ts root file. Thoughts?

[andrewbranch]

Further, it seems odd to me that triple-slash references, regardless of requesting resolution mode, wouldn’t use exports when both CJS- and ESM-mode imports do.

You might say that a triple-slash reference isn’t an import, so it doesn’t seem obvious that it should care about exports. But triple-slash references have always pulled from the types field which is analogous to the main field, which is the place where a require of the package name resolves to. So extending that analogy, it seems odd not to look for exports["."] in a mode where a require/import would 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 a main or types at all.

[weswigham]

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]

Wait, why should it be independent of moduleResolution? It’s not independent of moduleResolution today because the secondary lookup does use the moduleResolutionState.features based on module resolution.

[weswigham]

I'm thinking we should probably have triple-slash refs always use, eg, nodenext rules, 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]

I don’t think so

TypeScript/src/compiler/moduleNameResolver.ts

Line 359 in 1592210

let features = getDefaultNodeResolutionFeatures(options);

[weswigham]

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]

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 the moduleResolution setting, we will need to pick a set of conditions and whether NodeResolutionFeatures.EsmMode is set in modes where those are otherwise meaningless.

[weswigham]

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 || require list. 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 the moduleResolution doesn'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 when moduleResolution is a nodeish one, to get that to work.

[andrewbranch]

For the proposed minimal and hybrid, I’m fairly comfortable with saying it’s always import. For node, 🤷?

[weswigham]

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.