Skip to content

Watch failed lookups, affecting locations only if resolution was failed #61861

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sheetalkamat
Copy link
Member

This is the most aggressive version : 3 types of tests failure

  1. edits to package.json are ignored - most scenarios these edits will be watched for files's mode
  2. alternate js result to report error is not watched - but that changing and hence change to error message is almost not a normal scenario
  3. file was resolved in a node_modules folder but it appears in node_modules somewhere we would have found before that node_modules lookup - restart tsserver as this seems uncommon scenario,
    Left the tests to fail

Number of watched resolutions went down!! so invalidating time should go down drastically

Info 17690[12:45:13.042] Finishing updateGraphWorker: Project: tsconfig.json projectStateVersion: 1 projectProgramVersion: 0 structureChanged: true structureIsReused:: Not Elapsed: 47915.4494ms
Info 17691[12:45:13.043] ResolutionCache:: {
 "resolutionsWithFailedLookups": 14105,
 "resolutionsWithOnlyAffectingLocations": 4,
 "directoryWatchesOfFailedLookups": 1288,
 "fileWatchesOfAffectingLocations": 3157,
 "resolvedFileToResolution": 11309
}

to

Info 13556[13:26:19.042] Finishing updateGraphWorker: Project: tsconfig.json projectStateVersion: 1 projectProgramVersion: 0 structureChanged: true structureIsReused:: Not Elapsed: 42156.215200000006ms
Info 13557[13:26:19.042] ResolutionCache:: {
 "resolutionsWithFailedLookups": 151,
 "resolutionsWithOnlyAffectingLocations": 0,
 "directoryWatchesOfFailedLookups": 20,
 "fileWatchesOfAffectingLocations": 2796,
 "resolvedFileToResolution": 11309
}

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 13, 2025
@sheetalkamat
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
pack this ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2025

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/165350/artifacts?artifactName=tgz&fileId=2771ED305D29E25E36FCABC5F9002D96578FDF58FFA22A77769BA30DF2B9868C02&fileName=/typescript-5.9.0-insiders.20250613.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.9.0-pr-61861-2".;

@sheetalkamat
Copy link
Member Author

@typescript-bot cherry-pick this to release-5.8

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 13, 2025

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.8 ✅ Started ✅ Results

@typescript-bot
Copy link
Collaborator

Hey, @sheetalkamat! I've created #61864 for you. This involved updating baselines; please check the diff.

@DanielRosenwasser
Copy link
Member

So, for anyone reading this, and my own understanding - the problem is that typically if you do an import of foo, we have to run through many different possible locations to resolve a file. However, even if we find the module, we will still have to install file watchers for all of the locations we looked at before successfully resolving. This is to accurately capture some new file coming into existence and taking precedence. That might be an npm install on an inner directory, or a file with a "higher priority" extension, or something else entirely.

But that is generally uncommon if we've actually resolved a module. And so it sounds like the idea here is that if a module was actually resolved and found, we won't add watchers for any place we would have looked. If you do find yourself in an uncommon situation, you'll need to restart tsserver.

@DanielRosenwasser
Copy link
Member

edits to package.json are ignored - most scenarios these edits will be watched for files's mode

Why does this happen anyway? Shouldn't this trigger a re-resolve?

alternate js result to report error is not watched - but that changing and hence change to error message is almost not a normal scenario

This one I'm not clear on - can you elaborate?

'package.json' does not have a 'peerDependencies' field.
Resolving real path for '/user/username/projects/myproject/node_modules/pkg2/build/other.d.ts', result '/user/username/projects/myproject/packages/pkg2/build/other.d.ts'.
======== Module name 'pkg2' was successfully resolved to '/user/username/projects/myproject/packages/pkg2/build/other.d.ts' with Package ID 'pkg2/build/other.d.ts@1.0.0'. ========
packages/pkg1/index.ts:1:15 - error TS2305: Module '"pkg2"' has no exported member 'TheNum'.
Copy link
Member Author

Choose a reason for hiding this comment

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

Package.json was watched in two scenarios before this change -

  1. if package json was used to determine file's metadata - in node16 etc
  2. if this affected already resolved module resolution

Because we arent doing 2 watch any more and node10 isnt using package.json - this discrepency is shown in the test which uses node10

@@ -998,7 +801,7 @@ Info seq [hh:mm:ss:mss] event:
"line": 2,
"offset": 26
},
"text": "Could not find a declaration file for module 'bar'. '/home/src/projects/project/node_modules/bar/index.mjs' implicitly has an 'any' type.\n Try `npm i --save-dev @types/bar` if it exists or add a new declaration (.d.ts) file containing `declare module 'bar';`",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the error message given if module resolution fails: and then the message changes based of if we can find the resolution using node10 or not. With not watching this location (the resolution was to ".js" file) we wil not change the error based on whether you later install @types which is incorrectly typed

@sheetalkamat
Copy link
Member Author

edits to package.json are ignored - most scenarios these edits will be watched for files's mode

Why does this happen anyway? Shouldn't this trigger a re-resolve?

3eb3e21#r2152786800
This shows example of when it doesnt retrigger re-resolve. I can tweak it to watch it if its node10

alternate js result to report error is not watched - but that changing and hence change to error message is almost not a normal scenario

This one I'm not clear on - can you elaborate?

3eb3e21#r2152817116 This shows example and how this comes into play with what error is displayed

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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants