Include type reference directives in symlink cache, wait until program is present to create it #44259
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 3794e04. You can monitor the build here. |
|
It seems like the packing has failed |
|
friendly ping @andrewbranch it failed to build |
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at a522672. You can monitor the build here. |
| @@ -1594,6 +1594,7 @@ namespace ts.Completions { | |||
|
|
|||
| function tryGetImportCompletionSymbols(): GlobalsSearch { | |||
| if (!importCompletionNode) return GlobalsSearch.Continue; | |||
| isNewIdentifierLocation = true; | |||
There was a problem hiding this comment.
I discovered this was missing only because I added fourslash server tests... unrelated to main change
|
Hey @andrewbranch, 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 |
I have concern with this.. |
|
My guess is it’s not broken because if we fail to cache or throw away that initially cached info, we eventually fall back to |
|
With outDir and file not being built in referenced project might be good scenario to look at (with symlinks obviously) ? |
|
this version fixes the problems in our project. thanks! |
|
|
||
| if (!preferSymlinks) { | ||
| // Symlinks inside ignored paths are already filtered out of the symlink cache, | ||
| // so we only need to remove them from the realpath filenames. | ||
| const result = forEach(targets, p => !(shouldFilterIgnoredPaths && containsIgnoredPath(p)) && cb(p, referenceRedirect === p)); | ||
| if (result) return result; | ||
| } | ||
| const links = host.getSymlinkCache | ||
| ? host.getSymlinkCache() | ||
| : discoverProbableSymlinks(host.getSourceFiles(), getCanonicalFileName, cwd); |
There was a problem hiding this comment.
While refactoring for better preservation of a single SymlinkCache instance, I decided to get rid of this fallback. ModuleSpecifierResolutionHost is internal and all our implementations of it implement getSymlinkCache. It always delegates to Program.getSymlinkCache which delegates to Project.getSymlinkCache, but Program itself has a fallback. This should never be called directly by public API consumers, and even those who are using internal things should be taken care of via ts.createModuleSpecifierResolutionHost, so I think this is safe to remove.
| assert.deepEqual( | ||
| project.getSymlinkCache()?.getSymlinkedDirectories()?.get(link.path + "/" as Path), | ||
| { real: "/packages/dep/", realPath: "/packages/dep/" as Path } |
There was a problem hiding this comment.
This proves that directoryExists calls during program creation stayed in the cache because the /app/packages/tsconfig.json project has no source files with successful resolutions to the symlinked project, but it did discover the symlink as it tried to resolve import "dep/does/not/exist".
|
We had an issue in TS 4.3 with auto imports suggesting long relative paths in a monorepo using Yarn workspaces. This PR seems to fix it! 🎉 TS 4.3.2 TS 4.2.4, this PR |
Fixes a half-regression half-new-issue noted in #40584 (comment) where (some) auto imports and fixes inside
node_modules/.pnpmwere showing up again. The bug was surfaced by #43149, but only through a coincidental change in order of operations. The real root causes were:Projectwas creating an empty symlink cache too early—by deferring until after program creation, it can seed the cache with the symlinks discovered during module resolution for the files in that program.While testing, I noticed that it was still possible to get import statement completions with
.pnpmmodule specifiers for transitive dependencies, which are not symlinked into your own node_modules. When I first wrote the.pnpmexclusion stuff, I allowed the.pnpmrealpath to be used as a last resort if no suitable symlinks were found, primarily to avoid breaking declaration emit, which (naturally) assumes that you can always name a relative path from one module to another. I now allow the module specifier resolution host used by the services layer to opt out of getting these last-resort paths, so it can filter these suggestions out of its auto-imports.