microsoft
Proposal: Prioritize declaration file resolutions when inside node_modules
Reverses the priority order of .ts/.d.ts in node_modules so we prefer loading declaration files from libraries when .ts and .d.ts files are colocated.
We have encouraged libraries to consider shipping their source .ts files and declaration maps to npm to improve the user experience of things like go-to-definition. However, this has problems if the library didn’t use an outDir or declarationDir to separate their declaration files from source files. It’s more expensive for us to load .ts files, and it can cause checking errors like the ones in #58353, where globals only needed for checking implementations are missing, or those implementations have checker errors due to the user’s compiler options.
I think the biggest risk here is that it somehow adversely affects monorepos with node_modules symlinks. However, when used with project references, a different mechanism handles remapping from declaration files to implementation source files or vice versa, depending on settings and whether the program is being built on the command line or as part of editor services. Some monorepo users don’t use project references, but I think in that case, they’re also not generating declaration files. Additionally, most people use an outDir. (JSR’s reason for not wanting to compile into an outDir is it changes the runtime structure of import.meta.url from what the author expected, from the perspective of authoring in, and only ever thinking about, .ts files.) I think this is ok, but @sheetalkamat may know of extra things that need to be handled for monorepos / project references, or may think of reasons why this is a bad idea generally.
Closes #58353
Starting jobs; this comment will be updated as builds start and complete.
| Command | Status | Results |
|---|---|---|
test top400 |
✅ Started | 👀 Results |
user test this |
✅ Started | ✅ Results |
run dt |
✅ Started | ✅ Results |
perf test this faster |
✅ Started | 👀 Results |
@andrewbranch Here are the results of running the user tests comparing main and refs/pull/58553/merge:
Everything looks good!
I think main concern was mixing input and output files in. Even with this PR you can run into this.
Eg structure:
// /repos/node_modules/foo symlinked to /repos/foo
// /repos/node_modules/foo/index.d.ts
import { y } from "./y";
// /repos/node_modules/foo/index.ts
import { y } from "./y";
// /repos/node_modules/foo/y.ts
import { something } from "./index";
export const y = 10;
// /repos/node_modules/foo/y.d.ts
export const y = 10;
// /repos/bar/src/index.ts
import {something} from "foo";
With your PR we will include /repos/foo/index.d.ts from bar (though node resolution but if there is no preserveSymlinks = value of this is false by default so always will resolve to actual file path) and any imports in this will now resolve to ts files preference since they dont have node_modules in them and we end up getting foo/index.d.ts and foo/inded.ts as well in the program.
@andrewbranch The results of the perf run you requested are in!
Here they are:
tsc
Comparison Report - baseline..pr| Metric | baseline | pr | Delta | Best | Worst | p-value |
|---|---|---|---|---|---|---|
| Compiler-Unions - node (v18.15.0, x64) | ||||||
| Errors | 30 | 30 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 62,154 | 62,154 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 50,248 | 50,248 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 192,210k (± 0.01%) | 192,308k (± 0.08%) | ~ | 192,169k | 192,591k | p=0.128 n=6 |
| Parse Time | 1.30s (± 1.34%) | 1.28s (± 1.08%) | -0.03s (- 2.17%) | 1.26s | 1.29s | p=0.023 n=6 |
| Bind Time | 0.72s | 0.72s | ~ | ~ | ~ | p=1.000 n=6 |
| Check Time | 9.54s (± 0.15%) | 9.54s (± 0.40%) | ~ | 9.50s | 9.61s | p=0.292 n=6 |
| Emit Time | 2.65s (± 1.08%) | 2.64s (± 1.15%) | ~ | 2.60s | 2.68s | p=0.629 n=6 |
| Total Time | 14.21s (± 0.15%) | 14.17s (± 0.52%) | ~ | 14.11s | 14.30s | p=0.182 n=6 |
| angular-1 - node (v18.15.0, x64) | ||||||
| Errors | 5 | 5 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 944,110 | 944,110 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 407,140 | 407,140 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 1,222,139k (± 0.01%) | 1,221,666k (± 0.00%) | -474k (- 0.04%) | 1,221,580k | 1,221,713k | p=0.005 n=6 |
| Parse Time | 6.79s (± 0.75%) | 6.81s (± 0.70%) | ~ | 6.74s | 6.86s | p=0.572 n=6 |
| Bind Time | 1.88s (± 0.48%) | 1.88s (± 0.34%) | ~ | 1.87s | 1.89s | p=1.000 n=6 |
| Check Time | 31.34s (± 0.57%) | 31.39s (± 0.55%) | ~ | 31.22s | 31.70s | p=0.378 n=6 |
| Emit Time | 14.74s (± 0.43%) | 14.76s (± 0.66%) | ~ | 14.66s | 14.90s | p=1.000 n=6 |
| Total Time | 54.75s (± 0.45%) | 54.84s (± 0.35%) | ~ | 54.58s | 55.16s | p=0.378 n=6 |
| mui-docs - node (v18.15.0, x64) | ||||||
| Errors | 5 | 5 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,964,178 | 1,964,178 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 819,287 | 819,287 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 1,849,624k (± 0.00%) | 1,847,524k (± 0.00%) | -2,100k (- 0.11%) | 1,847,493k | 1,847,554k | p=0.005 n=6 |
| Parse Time | 6.77s (± 0.35%) | 6.77s (± 0.32%) | ~ | 6.74s | 6.80s | p=0.513 n=6 |
| Bind Time | 2.31s (± 1.05%) | 2.30s (± 0.81%) | ~ | 2.28s | 2.32s | p=0.866 n=6 |
| Check Time | 58.60s (± 0.40%) | 58.51s (± 0.51%) | ~ | 58.15s | 59.03s | p=0.521 n=6 |
| Emit Time | 0.14s (± 2.88%) | 0.14s (± 3.60%) | ~ | 0.14s | 0.15s | p=0.595 n=6 |
| Total Time | 67.82s (± 0.34%) | 67.72s (± 0.44%) | ~ | 67.37s | 68.24s | p=0.471 n=6 |
| self-build-src - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,221,231 | 1,221,232 | +1 (+ 0.00%) | ~ | ~ | p=0.001 n=6 |
| Types | 259,523 | 259,523 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,337,070k (± 0.02%) | 2,337,902k (± 0.03%) | +831k (+ 0.04%) | 2,336,902k | 2,338,615k | p=0.045 n=6 |
| Parse Time | 5.02s (± 1.16%) | 5.01s (± 0.59%) | ~ | 4.96s | 5.04s | p=0.872 n=6 |
| Bind Time | 1.89s (± 0.80%) | 1.88s (± 0.80%) | ~ | 1.87s | 1.90s | p=0.933 n=6 |
| Check Time | 33.82s (± 0.25%) | 33.71s (± 0.20%) | -0.10s (- 0.31%) | 33.63s | 33.78s | p=0.025 n=6 |
| Emit Time | 2.67s (± 1.62%) | 2.60s (± 2.61%) | ~ | 2.51s | 2.68s | p=0.065 n=6 |
| Total Time | 43.42s (± 0.24%) | 43.22s (± 0.16%) | -0.20s (- 0.46%) | 43.14s | 43.33s | p=0.008 n=6 |
| self-build-src-public-api - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 1,221,231 | 1,221,232 | +1 (+ 0.00%) | ~ | ~ | p=0.001 n=6 |
| Types | 259,523 | 259,523 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,413,882k (± 0.03%) | 2,413,546k (± 0.03%) | ~ | 2,412,544k | 2,414,204k | p=0.378 n=6 |
| Parse Time | 7.78s (± 0.79%) | 7.72s (± 0.99%) | ~ | 7.62s | 7.84s | p=0.128 n=6 |
| Bind Time | 2.51s (± 0.91%) | 2.50s (± 1.09%) | ~ | 2.46s | 2.54s | p=0.688 n=6 |
| Check Time | 49.94s (± 0.61%) | 49.75s (± 0.17%) | ~ | 49.64s | 49.86s | p=0.128 n=6 |
| Emit Time | 3.91s (± 2.49%) | 3.98s (± 4.36%) | ~ | 3.81s | 4.28s | p=0.378 n=6 |
| Total Time | 64.14s (± 0.68%) | 63.98s (± 0.39%) | ~ | 63.77s | 64.39s | p=0.378 n=6 |
| self-compiler - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 256,768 | 256,769 | +1 (+ 0.00%) | ~ | ~ | p=0.001 n=6 |
| Types | 104,587 | 104,587 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 426,070k (± 0.01%) | 426,135k (± 0.01%) | ~ | 426,046k | 426,212k | p=0.093 n=6 |
| Parse Time | 3.38s (± 0.81%) | 3.37s (± 0.91%) | ~ | 3.34s | 3.42s | p=0.936 n=6 |
| Bind Time | 1.32s (± 0.78%) | 1.33s (± 0.31%) | ~ | 1.32s | 1.33s | p=0.055 n=6 |
| Check Time | 17.93s (± 0.34%) | 17.92s (± 0.23%) | ~ | 17.87s | 17.98s | p=0.871 n=6 |
| Emit Time | 1.36s (± 1.20%) | 1.38s (± 2.17%) | ~ | 1.34s | 1.42s | p=0.418 n=6 |
| Total Time | 23.99s (± 0.27%) | 24.00s (± 0.18%) | ~ | 23.96s | 24.08s | p=0.809 n=6 |
| ts-pre-modules - node (v18.15.0, x64) | ||||||
| Errors | 35 | 35 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 224,575 | 224,575 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 93,785 | 93,785 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 369,821k (± 0.02%) | 369,898k (± 0.04%) | ~ | 369,737k | 370,100k | p=0.689 n=6 |
| Parse Time | 2.84s (± 1.06%) | 2.86s (± 1.15%) | ~ | 2.81s | 2.89s | p=0.290 n=6 |
| Bind Time | 1.58s (± 0.52%) | 1.57s (± 0.87%) | ~ | 1.56s | 1.60s | p=0.325 n=6 |
| Check Time | 15.68s (± 0.46%) | 15.61s (± 0.32%) | ~ | 15.55s | 15.66s | p=0.090 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 20.08s (± 0.46%) | 20.04s (± 0.18%) | ~ | 20.01s | 20.09s | p=0.520 n=6 |
| vscode - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 2,823,415 | 2,823,415 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 957,881 | 957,881 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 2,996,493k (± 0.00%) | 2,996,298k (± 0.00%) | -195k (- 0.01%) | 2,996,205k | 2,996,350k | p=0.005 n=6 |
| Parse Time | 13.81s (± 0.29%) | 13.85s (± 0.44%) | ~ | 13.78s | 13.94s | p=0.376 n=6 |
| Bind Time | 4.18s (± 2.14%) | 4.14s (± 0.25%) | ~ | 4.13s | 4.15s | p=0.209 n=6 |
| Check Time | 73.60s (± 0.30%) | 73.17s (± 0.42%) | ~ | 72.83s | 73.53s | p=0.066 n=6 |
| Emit Time | 23.60s (± 0.65%) | 23.52s (± 0.60%) | ~ | 23.31s | 23.73s | p=0.423 n=6 |
| Total Time | 115.18s (± 0.18%) | 114.67s (± 0.29%) | -0.51s (- 0.44%) | 114.10s | 114.97s | p=0.013 n=6 |
| webpack - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 265,866 | 265,866 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 108,401 | 108,401 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 410,499k (± 0.01%) | 410,488k (± 0.01%) | ~ | 410,438k | 410,531k | p=0.575 n=6 |
| Parse Time | 3.84s (± 1.18%) | 3.83s (± 0.85%) | ~ | 3.80s | 3.88s | p=0.747 n=6 |
| Bind Time | 1.67s (± 1.35%) | 1.67s (± 1.23%) | ~ | 1.65s | 1.70s | p=0.677 n=6 |
| Check Time | 16.92s (± 0.16%) | 16.93s (± 0.34%) | ~ | 16.88s | 17.03s | p=0.935 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 22.43s (± 0.30%) | 22.44s (± 0.33%) | ~ | 22.35s | 22.53s | p=0.936 n=6 |
| xstate-main - node (v18.15.0, x64) | ||||||
| Errors | 0 | 0 | ~ | ~ | ~ | p=1.000 n=6 |
| Symbols | 524,639 | 524,639 | ~ | ~ | ~ | p=1.000 n=6 |
| Types | 178,906 | 178,906 | ~ | ~ | ~ | p=1.000 n=6 |
| Memory used | 462,662k (± 0.02%) | 462,347k (± 0.02%) | -315k (- 0.07%) | 462,291k | 462,551k | p=0.008 n=6 |
| Parse Time | 3.88s (± 0.51%) | 3.85s (± 0.28%) | -0.03s (- 0.77%) | 3.83s | 3.86s | p=0.022 n=6 |
| Bind Time | 1.44s (± 0.85%) | 1.45s (± 1.27%) | ~ | 1.43s | 1.47s | p=0.869 n=6 |
| Check Time | 22.58s (± 0.47%) | 22.47s (± 0.65%) | ~ | 22.27s | 22.66s | p=0.199 n=6 |
| Emit Time | 0.00s | 0.00s | ~ | ~ | ~ | p=1.000 n=6 |
| Total Time | 27.91s (± 0.37%) | 27.77s (± 0.46%) | ~ | 27.61s | 27.96s | p=0.108 n=6 |
- node (v18.15.0, x64)
- Compiler-Unions - node (v18.15.0, x64)
- angular-1 - node (v18.15.0, x64)
- mui-docs - node (v18.15.0, x64)
- self-build-src - node (v18.15.0, x64)
- self-build-src-public-api - node (v18.15.0, x64)
- self-compiler - node (v18.15.0, x64)
- ts-pre-modules - node (v18.15.0, x64)
- vscode - node (v18.15.0, x64)
- webpack - node (v18.15.0, x64)
- xstate-main - node (v18.15.0, x64)
| Benchmark | Name | Iterations |
|---|---|---|
| Current | pr | 6 |
| Baseline | baseline | 6 |
Developer Information:
Hey @andrewbranch, the results of running the DT tests are ready.
Everything looks the same!
/repos/node_modules/foo symlinked to /repos/foo
I don’t think it’s realistic for a realpath in node_modules to be symlinked to a location outside node_modules; every real scenario I’ve ever seen has either been the opposite (e.g. npm link ../foo or monorepo workspaces) or both the realpath and symlink location are inside node_modules (e.g. pnpm).
/repos/node_modules/foo symlinked to /repos/fooI don’t think it’s realistic for a realpath in
node_modulesto be symlinked to a location outsidenode_modules; every real scenario I’ve ever seen has either been the opposite (e.g.npm link ../fooor monorepo workspaces) or both the realpath and symlink location are inside node_modules (e.g. pnpm).
This is pretty common: cd foo npm link cd ../bar npm link foo
- this creates bar/node_modules/foo whose real path is "../foo" .. The first resolution inside foo will take place with "node_modules" in it but resolutions inside that file will be done for "foo" as the path is resolved for the file to real path
@andrewbranch Here are the results of running the top 400 repos comparing main and refs/pull/58553/merge:
Something interesting changed - please have a look.
Details
honojs/hono
5 of 6 projects failed to build with the old tsc and were ignored
tsconfig.json
-
error TS2306: File '/mnt/ts_downloads/_/m/hono/node_modules/@cloudflare/workers-types/index.d.ts' is not a module. -
error TS7006: Parameter 'evt' implicitly has an 'any' type. -
error TS2578: Unused '@ts-expect-error' directive. -
error TS2352: Conversion of type 'SupportedElement' to type 'HTMLSelectElement' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first. -
error TS2322: Type 'SupportedElement | Text' is not assignable to type 'ChildNode | null'. -
error TS2345: Argument of type 'SupportedElement | Text | undefined' is not assignable to parameter of type 'ChildNode | null | undefined'. -
error TS18046: 'data' is of type 'unknown'.- src/middleware/method-override/index.test.ts#L29
- src/middleware/method-override/index.test.ts#L30
- src/middleware/method-override/index.test.ts#L31
- src/middleware/method-override/index.test.ts#L44
- src/middleware/method-override/index.test.ts#L45
- src/middleware/method-override/index.test.ts#L46
- src/middleware/method-override/index.test.ts#L75
- src/middleware/method-override/index.test.ts#L76
- src/middleware/method-override/index.test.ts#L77
- src/middleware/method-override/index.test.ts#L93
- src/middleware/method-override/index.test.ts#L94
- src/middleware/method-override/index.test.ts#L95
- src/validator/validator.test.ts#L169
- src/validator/validator.test.ts#L170
- src/validator/validator.test.ts#L183
- src/validator/validator.test.ts#L184
nextauthjs/next-auth
18 of 40 projects failed to build with the old tsc and were ignored
packages/adapter-d1/tsconfig.json
-
error TS2306: File '/mnt/ts_downloads/_/m/next-auth/node_modules/.pnpm/@[email protected]/node_modules/@cloudflare/workers-types/index.d.ts' is not a module.
@cloudflare/workers-types ships an index.ts and index.d.ts side-by-side with different content, apparently used for different purposes 😐
@sheetalkamat I see your point now. That would be solved with something like this:
- const prioritizeDeclarationFiles = pathContainsNodeModules(candidate);
+ const prioritizeDeclarationFiles = pathContainsNodeModules(preserveSymlinks ? candidate : realPath(candidate));
if it doesn’t have an unacceptable perf impact. Would that address your concern, or do you have another suggestion, or do you think we shouldn’t try to do this?
That might work but again creates different behavior based on whether you are using npm link or not right. two reasons you could be doing npm link is:
- developing two packages at a time
- Or just testing out update to dependency - in this case you probably want the "d.ts" as if it wasn't npm linked.
So this becomes muddy.
real path on candidate is probably going to cause perf impact with looking many places for real path - but may be somehow combining with "record only failure flag" may work.
I think what we have currently is better because we just dont care where we are and follow the exact predefined steps. Its predictable.
We have these known issues with node_modules etc but they are known and there are work arounds for package managers.
Another such example of behaviour is allowJs, though it could be predictable behavior to change, it is better i think that we always prefere d.ts over ".js" .. It might not be correct behavior in all possible configurations but making them undesirable and unsupported configuration is better than having to deal with "your" vs "mine" code and other fall outs from that one.
Long story short I think we should leave the resolution priority as is. Just my personal opinion.
If .d.ts is preferred for resolution over .ts, wouldn’t that prevent GTD (the entire reason to ship .ts files in the first place) from working anyway?
Is it feasible to detect and track at a given resolution site that at least one resolution of a bare specifier (e.g. '@rushstack/node-core-library') has occurred to get to the current file? Essentially it would be a flag that once you have performed such a resolution, you prefer .d.ts files regardless of the path to the file.
