Adjustments to VSCode <-> DevTools communication #197
Conversation
| if (data.source === "vscode-panel") { | ||
| if (data.type === "mounted") { | ||
| sendToDevTools({ | ||
| type: "initializePanel", | ||
| initialContext: { | ||
| port: | ||
| serverState?.port || | ||
| vscode.workspace | ||
| .getConfiguration("apollographql") | ||
| .get("devTools.serverPort", 0), | ||
| listening: !!serverState?.port, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Where previously the panel kickstarted itself, it now notifies the extension, and the extension dispatches the initializePanel message with some additional context.
| | { port: false | number; disposable: Disposable } | ||
| | undefined = undefined; | ||
|
|
||
| export function startServer(port: number) { |
There was a problem hiding this comment.
A bunch of checks moved over from extension.ts here and the whole "disposing" and "restarting" should be a lot more solid now.
| | { port: false | number; disposable: Disposable } | ||
| | undefined = undefined; |
There was a problem hiding this comment.
false if the server is in the middle of starting, number if the server is running. The whole serverState is undefined after the server has stopped or if it has never been started before.
There was a problem hiding this comment.
Out of curiosity, why use false instead of null or undefined to represent the initial state?
There was a problem hiding this comment.
It felt more deliberate than undefined. null would have been fine too, I guess.
phryneas
force-pushed
the
pr/port-info
branch
from
9d0d030 to
83d67b2
Compare
|
You can download the latest build of the extension for this PR here: To install the extension, download the file, unzip it and install it in VS Code by selecting "Install from VSIX..." in the Extensions view. Alternatively, run code --install-extension vscode-apollo-0.0.0-build-1733911509.pr-197.commit-51db1cf.vsix --forcefrom the command line. For older builds, please see the edit history of this comment. |
phryneas
force-pushed
the
pr/port-info
branch
from
9cbe18c to
277a5ad
Compare
| if (data.source === "vscode-panel") { | ||
| if (data.type === "mounted") { |
There was a problem hiding this comment.
| if (data.source === "vscode-panel") { | |
| if (data.type === "mounted") { | |
| if (data.source === "vscode-panel" && data.type === "mounted") { |
You should be able to combine these conditionals.
There was a problem hiding this comment.
I assume that inside of data.source we'll see more reactions to other data.type in the future, which is why I separated those from each other.
| | { port: false | number; disposable: Disposable } | ||
| | undefined = undefined; |
There was a problem hiding this comment.
Out of curiosity, why use false instead of null or undefined to represent the initial state?
| | undefined = undefined; | ||
|
|
||
| export function startServer(port: number) { | ||
| const state = { |
There was a problem hiding this comment.
I'm curious, whats the advantage of using a separate local state variable here instead of just assigning this object to serverState and modifying that in wss.on('listening') for example? Is it to avoid ? or ! since serverState is of type | undefined?
There was a problem hiding this comment.
See a bit further down: if (serverState === state) { - it makes it possible to detect if serverState has been reassigned.
src/devtools/server.ts
Outdated
| if (serverState) { | ||
| if (serverState.port === port) { | ||
| // nothing to do | ||
| return; | ||
| } else { | ||
| // changing port, stop the old server | ||
| serverState.disposable.dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if (serverState) { | |
| if (serverState.port === port) { | |
| // nothing to do | |
| return; | |
| } else { | |
| // changing port, stop the old server | |
| serverState.disposable.dispose(); | |
| } | |
| } | |
| if (serverState && serverState.port !== port) { | |
| // changing port, stop the old server | |
| serverState.disposable.dispose(); | |
| } |
Should be able to simplify this by combining the conditionals and getting rid of the branch that doesn't do anything.
There was a problem hiding this comment.
I just noticed the return keyword 🤦. Ignore the original suggestion.
That said, I think just adding an if above this makes sense:
if (serverState?.port === port) {
return;
}Which would mean that any code after this would mean that serverState.port is different or uninitialized. I think then you can just remove the if. The whole thing would look like:
if (serverState?.port === port) {
return;
}
// changing port, stop the old server
serverState?.disposable.dispose();
// ...There was a problem hiding this comment.
and d4a65ee ... that took a moment to click for me
This is the sister PR for apollographql/apollo-client-devtools#1513
CI fails because no devtools package has been published yet, but please review it anyways :)