-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot pack this |
Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@typescript-bot cherry-pick this to release-5.8 |
Hey, @sheetalkamat! I've created #61864 for you. This involved updating baselines; please check the diff. |
So, for anyone reading this, and my own understanding - the problem is that typically if you do an import of 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. |
Why does this happen anyway? Shouldn't this trigger a re-resolve?
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/[email protected]'. ======== | ||
[96mpackages/pkg1/index.ts[0m:[93m1[0m:[93m15[0m - [91merror[0m[90m TS2305: [0mModule '"pkg2"' has no exported member 'TheNum'. |
There was a problem hiding this comment.
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 -
- if package json was used to determine file's metadata - in node16 etc
- 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';`", |
There was a problem hiding this comment.
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
3eb3e21#r2152786800
3eb3e21#r2152817116 This shows example and how this comes into play with what error is displayed |
I know this came from an internal repro; do we know if they're actually using a non-node10 mode such that this would be the case? That and, if I have a package consisting solely of Do we know how many watches removed correspond to the three things you've listed? |
They use esnext for resolution. I didnt do the three numbers but if you would like, i would have to do builds to get those number.. the only numbers i got were mentioned in the description which shows how many resolutions are not watched any more as well as reduction in number of watches |
Unfortunately the links aren't directing me to the right section, and I'll be honest - it's not obvious what is changing from the diffs. With
were you saying that that's expected for all resolution? Or that only some tests are affected by this?
I'm still not clear on this. Are you just saying that we don't watch for the existence of a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sort of an example I was concerned about; is it okay for this to not watch node_modules
in any form?
This is the most aggressive version : 3 types of tests failure
Left the tests to fail
Number of watched resolutions went down!! so invalidating time should go down drastically
to