-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Fix bug: ATA should still install types if a package was resolved to a '.js' file #27347
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
Conversation
src/server/project.ts
Outdated
| if (file.resolvedModules) { | ||
| file.resolvedModules.forEach((resolvedModule, name) => { | ||
| // pick unresolved non-relative names | ||
| if ((!resolvedModule || !extensionIsTS(resolvedModule.extension)) && |
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.
You should also ignore json resolution ?
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.
I don't think this would come up, because this code will only apply to non-relative imports, and we only resolve to .json for relative imports with an explicit .json extension, right?
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.
no we resolve any json resolution with ".json" extension
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.
I can't get a global import to ever resolve to a '.json' file -- if I import "foo", it looks in node_modules/foo/index.ts, .tsx, .d.ts, .js, and .jsx, but not .json. If I import "foo.json" that looks for node_modules/foo.json/index.ts etc.
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.
it will resolve to "node_modules/foo/index.json" when you import "foo.json" or import of "foo/lib.json" will resolve to "node_modules/foo/lib.json"?
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.
Ah OK, "foo/lib.json" can import a json module. Shouldn't we still look for a type definition for the package though?
|
For the other part you also want to fix |
|
@sheetalkamat Good to merge this first part? |
|
I would have you do other part right here since these both changes are related and better to be handled in one change. |
| return primaryResult; | ||
| } | ||
| ======= | ||
| function resolveModuleName(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, redirectedReference?: ResolvedProjectReference): CachedResolvedModuleWithFailedLookupLocations { |
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.
Need to fix the merge conflict
|
Where are the other changes in the project? |
|
A lot of changes were moved to #28092 which we might want to merge first. |
|
I don't think we should merge this. This change needs to be separate and need to handle the ATA change in project and resolution cache. |
|
Here's what I used to reproduce the issue:
But in this branch:
|
|
Closing in favor of #28236, which doesn't require us to have watchers on the typesCache, which is redundant since typingsInstaller should handle that anyway. (Thanks @sheetalkamat for pointing that out.) |
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Partial fix for #27302
Doesn't completely fix the issue since the newly installed ATA @types package doesn't replace the resolution of the local JS package, even though it should (since that's what happens when you reload the editor).