Skip to content

Update symlink cache from AutoImportProvider resolution even if host project already contains the file via its realpath #46830

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

Merged

Conversation

andrewbranch
Copy link
Member

This fixes part of a bug I noticed while writing #46569, mentioned in the test comment there. An AutoImportProviderProject contains only files that the host Project does not. However, it’s possible, as in the test written here, that the main project only knows about a file via its realpath, but it also exists as a symlink inside node_modules. Because that symlink was mentioned in the package.json dependencies, we discover it as part of spinning up an AutoImportProviderProject. However, when we see that the SourceFile we resolved to is already contained by the host Project, we skip it entirely, discarding the knowledge of the symlink we found. This PR allows the AutoImportProviderProject to add that symlink knowledge to the host project’s cache before it decides not to add the file to itself. This way, the host Project is still responsible for generating auto-imports of that SourceFile, but it can come up with the correct module specifier for it using the symlink information discovered by the AutoImportProviderProject.

…project already contains the file via its realpath
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 16, 2021
//// { "dependencies": { "mylib": "file:packages/mylib" } }

// @Filename: /packages/mylib/package.json
//// { "name": "mylib", "version": "1.0.0", "main": "index.js", "types": "index" }
Copy link
Member Author

Choose a reason for hiding this comment

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

The other part of the bug is this—for this test to work, this package.json needs "types": "index" exactly—leaving it off or specifying any file extension breaks the resolution attempted by the AutoImportProviderProject. My attempt to leverage resolveTypeReferenceDirective for this has hit its limit; there probably needs to be a much more permissive module resolution function written specifically for this. I’ll follow up with a separate PR for that.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Your explanation makes sense and your code change looks consistent with your explanation.

@andrewbranch andrewbranch merged commit 009dd48 into microsoft:main Nov 17, 2021
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
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.

5 participants