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

Use node's ipc for desktop TypeScript server #46417

Closed
mjbvz opened this issue Oct 18, 2021 · 4 comments
Closed

Use node's ipc for desktop TypeScript server #46417

mjbvz opened this issue Oct 18, 2021 · 4 comments
Labels
API Relates to the public API for TypeScript Domain: TSServer Issues related to the TSServer Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Oct 18, 2021

On desktop, VS Code currently communicates with the TypeScript server by passing messages over stdio. These messages are in a json rcp style format

We should instead explore using node's built-in ipc support which would let the two processes communicate using .send and .on('message', ...)

Advantages

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 18, 2021

I have work in progress that makes this change on the VS Code side and on the TS side (although I'll need help finalizing the TS part of this)

Unfortunately we'll likely need to keep support for the exiting rpc mechanism for any older clients that upgrade to use the new TypeScript version

mjbvz added a commit to microsoft/vscode that referenced this issue Oct 18, 2021
mjbvz added a commit to mjbvz/TypeScript that referenced this issue Oct 18, 2021
For microsoft#46417

This lets us use Node's built-in ipc for passing messages to/from the typescript server instead of using stdio
@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 18, 2021

VS Code PR: microsoft/vscode#135341

TS PR: #46418

@DanielRosenwasser DanielRosenwasser added In Discussion Not yet reached consensus Suggestion An idea for TypeScript API Relates to the public API for TypeScript Domain: TSServer Issues related to the TSServer labels Oct 18, 2021
@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 19, 2021

@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 20, 2021

Not sure if this matters too much, but I believe switching to ipc would also let us easily transfer some data as binary. The obvious candidate is encodedSemanticClassifications-full, which currently returns a big array of numbers. This could be a Uint32Array instead

mjbvz added a commit to microsoft/vscode that referenced this issue Dec 18, 2021
DanielRosenwasser pushed a commit that referenced this issue Jan 6, 2022
* Use node ipc for TS Server

For #46417

This lets us use Node's built-in ipc for passing messages to/from the typescript server instead of using stdio

* Remove extra parse

* Add extra logging when using node IPC

* Split out to subclass

* Extract common writeMessage method

* Baseline accept
@DanielRosenwasser DanielRosenwasser added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Jan 6, 2022
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.6.0 milestone Jan 6, 2022
mjbvz added a commit to microsoft/vscode that referenced this issue Jan 10, 2022
* Use node ipc for communicating with TS Server

For microsoft/TypeScript#46417

* Don't use ipc for stdio of we're not using ipc

* Use node ipc for communicating with TS Server

Fixes #85565
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relates to the public API for TypeScript Domain: TSServer Issues related to the TSServer Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants