Skip to content

fix: improve typescript server path search for ts dependents lsp #3795

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
merged 2 commits into from
Apr 28, 2025

Conversation

fuegoio
Copy link
Contributor

@fuegoio fuegoio commented Apr 27, 2025

LSPs like volar (for VueJS), astro or mdx_analyzer rely on a path for the Typescript server they should use. But this path is wrongly assumed to be inside node_modules/typescript/lib starting from the nearest package.json file.

In fact, in case of monorepos / workspaces, with all package managers like npm, yarn or pnpm, some hoisting can take place. This means that the typescript node module will get move higher up in the tree to be shared between all projects and avoid duplication. The NodeJS module resolution works like this, by trying to find the module upward in the tree, one directory after another.

Therefore, it can happen a case like this:

monorepo-root
 - apps
 > - app-number-one
 >> - node_modules (without typescript)
 >> - package.json
 > - app-number-two
 >> - node_modules (without typescript)
 >> - package.json
 - node_modules (with typescript)
 > - typescript
 - package.json

I therefore adapted the Lua algorithm to match the one of NodeJS (and the one of the ts_ls server, that does it natively).

Even if some LSPs like volar going towards "hybrid modes" where they don't need to spawn their full TS server (because usually users will spawn it nevertheless for other projects), this is still required and needs to be correctly setup to work on the files they target.

I've also mutualized this logic at one place to avoid de-duplication, but feel free to correct me on that. I've created a simple typescript.lua file, imagining that we could have language specific utils for LSPs that need it.

@fuegoio fuegoio requested a review from glepnir as a code owner April 27, 2025 17:53
@fuegoio
Copy link
Contributor Author

fuegoio commented Apr 27, 2025

Seeing from the CI that I should maybe not update the legacies LSP configs, but I understood they were still used in older versions of Nvim. Feel free to tell me what I should update or not.

@justinmk
Copy link
Member

cc @Grahf0085

but I understood they were still used in older versions of Nvim

well, they're frozen. anyone on an older version can either upgrade or use the old configs.

@justinmk
Copy link
Member

would neovim/neovim#33485 address your use-case here?

@fuegoio
Copy link
Contributor Author

fuegoio commented Apr 27, 2025

cc @Grahf0085

but I understood they were still used in older versions of Nvim

well, they're frozen. anyone on an older version can either upgrade or use the old configs.

So I don't update them, even for fixes like this that could benefit from a backporting? I'm myself not yet using the new config as my config is a bit messy and I have yet to understand how to change my setup to enable ahah.

would neovim/neovim#33485 address your use-case here?

Unfortunately the root marker is different from this as here we could have a root that is successfully determined as app-number-one if for example this project has Vue or Astro installed (main requirement for the LSP to work), but the TS server could still be up one level or two.

Honestly LSPs should be in charge of doing that instead but I guess they want to allow flexibility on instanciation.

@fuegoio fuegoio force-pushed the improve-typescript-dependents-lsp branch from c125041 to 1d41d3c Compare April 27, 2025 18:10
@justinmk
Copy link
Member

So I don't update them, even for fixes like this that could benefit from a backporting?

can make an exception in this case.

@fuegoio fuegoio force-pushed the improve-typescript-dependents-lsp branch from 1d41d3c to f80337c Compare April 28, 2025 06:54
@fuegoio
Copy link
Contributor Author

fuegoio commented Apr 28, 2025

can make an exception in this case.

Nice thanks! Normally good on my side then :)

@justinmk justinmk merged commit 9c6bbb5 into neovim:master Apr 28, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants