Skip to content
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

LSP: Investigate 'auto-imports from npm dependencies' hack for perf issues #27700

Open
nayeemrmn opened this issue Jan 16, 2025 · 1 comment
Open
Labels
lsp related to the language server perf performance related

Comments

@nayeemrmn
Copy link
Collaborator

https://github.com/nayeemrmn/deno/blob/9dbb99a83cb599028aef662b23e13faf2df15297/cli/lsp/tsc.rs#L4315-L4317

We use this to hide the fact that these paths are in a /node_modules/ folder because tsc wasn't giving auto-import suggestions from them. Did this have a knockback effect on performance? Could we only apply this to direct dependencies? How does node/tsc give these suggestions?

@nayeemrmn nayeemrmn added lsp related to the language server perf performance related labels Jan 16, 2025
@nathanwhit
Copy link
Member

nathanwhit commented Jan 16, 2025

Investigated the cause (didn't look at perf), the issue is this https://github.com/microsoft/TypeScript/blob/e9738050cf896a9c08d1d3d911f304513f80b76a/src/services/exportInfoMap.ts#L436-L453

Specifically

startsWith(getCanonicalFileName(fromPath), toNodeModulesParent)

it's checking if the import is in the top level node_modules. For an npm style node_modules layout, this works fine (example below from a project at /home/node-modules-completion:

DEBUG JS - TSC: isImportablePath 
fromPath="file:///home/node-modules-completion/main.ts"
toPath="file:///home/node-modules-completion/node_modules/preact/hooks/src/index.d.ts"
globalCachePath=undefined
toNodeModules="file:///home/node-modules-completion/node_modules"
toNodeModulesParent="file:///home/node-modules-completion"
startsWith(getCanonicalFileName(fromPath), toNodeModulesParent) === true

but with the pnpm style layout, when you fully resolve the symlinks this doesn't work:

DEBUG JS - TSC: isImportablePath
fromPath="file:///home/node-modules-completion/main.ts"
toPath="file:///home/node-modules-completion/node_modules/.deno/[email protected]/node_modules/preact/hooks/src/index.d.ts"
globalCachePath=undefined
toNodeModules= "file:///home/node-modules-completion/node_modules/.deno/[email protected]/node_modules"
toNodeModulesParent="file:///home/node-modules-completion/node_modules/.deno/[email protected]"
startsWith(getCanonicalFileName(fromPath), toNodeModulesParent) === false

The reason the current workaround works is that we effectively force toNodeModules=undefined and toNodeModulesParent=undefined, which makes isImportablePath just always return true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp related to the language server perf performance related
Projects
None yet
Development

No branches or pull requests

2 participants