Proposal: Prioritize declaration file resolutions when inside node_modules #58553
Conversation
typescript-bot
added
the
For Milestone Bug
|
@typescript-bot test it |
|
@andrewbranch Here are the results of running the user tests comparing 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 |
|
@andrewbranch Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @andrewbranch, the results of running the DT tests are ready. Everything looks the same! |
I don’t think it’s realistic for a realpath in |
This is pretty common:
|
|
@andrewbranch Here are the results of running the top 400 repos comparing Something interesting changed - please have a look. Details
|
|
|
|
@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.
So this becomes muddy. I think what we have currently is better because we just dont care where we are and follow the exact predefined steps. Its predictable. Another such example of behaviour is Long story short I think we should leave the resolution priority as is. Just my personal opinion. |
|
If |
|
No, that mapping is requested explicitly in the server. |
|
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. |
Reverses the priority order of
.ts/.d.tsinnode_modulesso we prefer loading declaration files from libraries when.tsand.d.tsfiles are colocated.We have encouraged libraries to consider shipping their source
.tsfiles 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 anoutDirordeclarationDirto separate their declaration files from source files. It’s more expensive for us to load.tsfiles, 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 anoutDiris it changes the runtime structure ofimport.meta.urlfrom what the author expected, from the perspective of authoring in, and only ever thinking about,.tsfiles.) 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