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

[typescript] re-use Theia launching program for TS LS #5324

Merged
merged 1 commit into from
Jun 1, 2019

Conversation

marcdumais-work
Copy link
Contributor

When we launch the TypeScript Language Server, we start it using
the hardcoded executable "node". This works well in practice for
most cases. However when we have a packaged Electron application,
installed by a user, it could be that the environment does not have
"node" immediately available in its path, or not a proper version,
as needed by tsserver.

So why not reuse the node executable that was used to launch
the Theia application, to launch the LS? In the Electron case,
it will be the Electron executable bundled with the packaging.

Signed-off-by: Marc Dumais marc.dumais@ericsson.com

const command = 'node';
// Re-use the same tool used to launch Theia. e.g. for an Electron Theia packaging,
// this will be "electron" executable that is bundled with the application.
const command = process.argv[0];
Copy link
Member

Choose a reason for hiding this comment

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

process.execPath ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@akosyakov
Copy link
Member

Usually child_process.fork should be used for it.

When we launch the TypeScript Language Server, we start it using
the hardcoded executable "node". This works well in practice for
most cases. However when we have a packaged Electron application,
installed by a user, it could be that the environment does not have
"node" immediately available in its path, or not a proper version,
as needed by tsserver.

So why not reuse the node executable that was used to launch
the Theia application, to launch the LS? In the Electron case,
it will be the Electron executable bundled with the packaging.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
@marcdumais-work
Copy link
Contributor Author

Usually child_process.fork should be used for it.

Can you elaborate on this? It looks like the spawning of the LS process is delegated to the BaseLanguageServerContribution, and there we use our internal RawProcess.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, as @marcdumais-work said, here we delegate the spawning to the base class.

@akosyakov
Copy link
Member

@marcdumais-work I meant to use RawForkOptions, then the current node.js proces fork a new node.js process for the given module making sure that the same node is used.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

probably execPath will work for now too

@marcdumais-work marcdumais-work merged commit de45794 into master Jun 1, 2019
@marcdumais-work marcdumais-work deleted the ts-ls-node-reuse branch June 1, 2019 13:50
@kittaakos
Copy link
Contributor

As a JS/TS developer, I would feel extremely alien that Theia does not use the Node.js from the PATH (which is most of the time configured with nvm) but the bundled one. This PR is very very friendly with people who would like to develop TS/JS without Node.js on the PATH, but this is the minority, I would say.

Can you please give some details about the use-case, @marcdumais-work? Thanks!

So the correct logic should be the following:

  • We check if Node.js is on the PATH, if yes, use that.
  • Otherwise, we can be helpful and provide the fallback from the bundled electron one.

Thoughts?

Also, it was causing #5385.

@marcdumais-work
Copy link
Contributor Author

Hi @kittaakos ,

This PR is very very friendly with people who would like to develop TS/JS without Node.js on the PATH, but this is the minority, I would say.

Agreed.

Can you please give some details about the use-case

It's pretty simple: I thought that it would be better to have a working TypeScript LS vs not. I figured that tsserver would probably always work with whatever node ends-up being bundled with Electron. I did not think there would be side-effects, since this affects only the node used to launch tsserver but that may have been not completely correct: thinking about it, some other components can be launched from it: at least tslint.

Also, should tsserver not work well with the specific node bundled with Electron, I agree I would assume it's using the node in my path, which would make it more difficult to troubleshoot.

In any case, your suggestion above sounds good and should work fine.

@kittaakos
Copy link
Contributor

Thanks for your detailed response, @marcdumais-work.

I thought that it would be better to have a working TypeScript LS vs not

Pure kindness 👍, I like the idea.

which would make it more difficult to troubleshoot.

Only this.

suggestion above sounds good and should work fine.

Follow up: #5408

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.

4 participants