Repository navigation
Use node ipc for TS Server - #46418
Conversation
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
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| // 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}`); |
There was a problem hiding this comment.
I think the answer here might be yes, or at least the portion that formatMessage does in verboseLogging mode as a side effect.
|
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. |
|
Jake Bailey (@jakebailey) This new flow should only be enabled if the client passes in the |
| } | ||
|
|
||
| public send(msg: protocol.Message) { | ||
| if (useNodeIpc) { |
There was a problem hiding this comment.
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.).
There was a problem hiding this comment.
Good suggestion. Made the fix
|
You'll need to accept the baseline for the public API. It seems like I don't have rights to change this branch though. |
|
Daniel Rosenwasser (@DanielRosenwasser) When I run |
|
Maybe you're only doing a partial test-run? I think CI is correctly pointing out a change in 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; |
|
Thanks! After running full tests I as able to run |
|
Thanks Matt! |
6 participants
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