fix: ensure that the tsserver subprocess uses same node instance #292
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The support for passing path to a `tsserver` shell script or just a `tsserver` string to `--tsserver-path`, and relying it to be found on the PATH, is removed. The provided path must point to the `typescript/lib` directory (or `typescript/lib/tsserver.js` script for backward compatibility) and the server will run it through the `tsserver.js` path that it will look up. Starting `tsserver` through a shell script is wrong and in some cases even causes server to not work at all. That's because spawning a node process for `tsserver` instead of forking the current one can result in running a completely different node version or even a node version that is sandboxed and can't communicate with this LSP server. Fixes #191
This was referenced Nov 13, 2021
rchl
changed the title
This was referenced Nov 21, 2021
This was referenced Nov 25, 2021
itsfarseen
added a commit
to itsfarseen/home.nix
that referenced
this pull request
Jul 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The server has always supported two ways of starting the
tsserversub-process - either forking the current process or spawning a new
Node process. The latter was problematic in some cases and this PR
is removing support for that.
Forking the process was used when we had a path to the
tsserver.jsscript while spawning was used when we had either just a binary
name like
tsserveror a full path to the thetsservershellscript. Since a shell script can't be started directly by Node, we
had to let the shell handle it and start the Node process that way.
That was an issue since it sometimes resulted in starting a completely
different instance of Node found on the PATH, that was potentially not
compatible or, more likely, sandboxed as is the case with Snap packages.
This PR removes support for spawning a process but also adds logic
that will try to find the path to the corresponding
tsserver.jsscript if we've only been passed
tsserveror path to the shellscript through
--tsserver-path. This should ensure that the serverstays backward-compatible with the previous approach as much as
possible (even if not 100%).
Fixes #191