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 ipc for TS Server #46418

Merged
merged 6 commits into from
Jan 6, 2022
Merged

Conversation

mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Oct 18, 2021

For #46417

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

Needs review from the TS team because there are still a few todo comments and I'm pretty sure I've overlooked some things

For microsoft#46417

This lets us use Node's built-in ipc for passing messages to/from the typescript server instead of using stdio
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 18, 2021
@mjbvz mjbvz marked this pull request as ready for review November 29, 2021 11:51
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Comment on lines 726 to 728
// TODO: Do we need any perf stuff here?
// const msgText = formatMessage(msg, this.logger, this.byteLength, this.host.newLine);
// perfLogger.logEvent(`Response message size: ${msgText.length}`);
Copy link
Member

Choose a reason for hiding this comment

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

I think the answer here might be yes, or at least the portion that formatMessage does in verboseLogging mode as a side effect.

@jakebailey
Copy link
Member

The PR looks good to me as it is now (I don't see any gotchas), though I don't know if it's ready or if the VS Code side is done to test.

@mjbvz
Copy link
Contributor Author

mjbvz commented Dec 9, 2021

@jakebailey This new flow should only be enabled if the client passes in the --useNodeIpc flag so I it can be merged whenever the TS changes look good. I have a separate change to adopt this on the VS Code side

@@ -715,6 +715,42 @@ namespace ts.server {
this.constructed = true;
}

public send(msg: protocol.Message) {
if (useNodeIpc) {
Copy link
Member

Choose a reason for hiding this comment

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

Please consider refactoring. The repeated flag check is a good indicator that you actually want method overrides. Maybe we're looking at a third subclass of Session (WorkerSession, IOSession and maybe NodeIpcSession?)

parseMessage and toStringMessage are the same as in the WorkerSession implementation.

send seems to be different for all three implementations, but does it need to be? That msg.type === "event" check and logging code applies to all implementations so it may as well live in the base class. Then you probably want to pull out the writeMessage call into a protected method to make it specific to the subclass, since that's the part that actually differs between the three.

(I don't know if we want to follow these OOP patterns all across the codebase, but since the session code is written that way, my advice is following that pattern.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. Made the fix

@DanielRosenwasser
Copy link
Member

You'll need to accept the baseline for the public API. It seems like I don't have rights to change this branch though.

@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 3, 2022

@DanielRosenwasser When I run gulp baseline-accept I don't see any changes. Is that expected since this commit only changes the flags passed to the server and not any d.ts apis?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jan 4, 2022

Maybe you're only doing a partial test-run? I think CI is correctly pointing out a change in api/tsserverlibrary.d.ts.

         private projectsUpdatedInBackgroundEvent;
         logError(err: Error, cmd: string): void;
         private logErrorWorker;
         send(msg: protocol.Message): void;
+        protected writeMessage(msg: protocol.Message): void;
         event<T extends object>(body: T, eventName: string): void;
         /** @deprecated */
         output(info: any, cmdName: string, reqSeq?: number, errorMsg?: string): void;
         private doOutput;

@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 6, 2022

Thanks! After running full tests I as able to run baseline-accept

@DanielRosenwasser DanielRosenwasser merged commit 6966209 into microsoft:main Jan 6, 2022
@DanielRosenwasser
Copy link
Member

Thanks Matt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants