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

Adjustments to VSCode <-> DevTools communication #197

Merged
merged 6 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Adjustments to VSCode <-> DevTools communication
  • Loading branch information
phryneas committed Dec 6, 2024
commit 3700b50a9947632a28a20e32079d82a045dc0238
71 changes: 61 additions & 10 deletions src/devtools/DevToolsViewProvider.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,34 @@
import * as vscode from "vscode";
import { devtoolsEvents } from "./server";
import { devtoolsEvents, sendToDevTools, serverState } from "./server";

type ActorMessage = { type: "actor"; message: unknown };

export function isActorMessage(message: any): message is ActorMessage {
return message && message.type === "actor";
}

type DevToolsOpenCommandMessage = {
type: "vscode:executeCommand";
command: string;
arguments?: unknown[];
};

export function isDevToolsExecuteCommandMessage(
message: any,
): message is DevToolsOpenCommandMessage {
return message && message.type === "vscode:executeCommand";
}

type DevToolsOpenExternalMessage = {
type: "vscode:openExternal";
uri: string;
};

export function isDevToolsOpenExternalMessage(
message: any,
): message is DevToolsOpenExternalMessage {
return message && message.type === "vscode:openExternal";
}

export class DevToolsViewProvider implements vscode.WebviewViewProvider {
public static readonly viewType = "vscode-apollo-client-devtools";
Expand All @@ -20,16 +49,40 @@ export class DevToolsViewProvider implements vscode.WebviewViewProvider {
panel.webview,
this._extensionUri,
);

panel.webview.onDidReceiveMessage((data) => {
devtoolsEvents.emit("fromDevTools", data);
});
const panelDisposables: vscode.Disposable[] = [];
panel.webview.onDidReceiveMessage(
(data) => {
if (data.source === "apollo-client-devtools") {
devtoolsEvents.emit("fromDevTools", data);
}
if (data.source === "vscode-panel") {
if (data.type === "mounted") {
Comment on lines +58 to +59
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

sendToDevTools({
type: "initializePanel",
initialContext: {
port:
serverState?.port ||
vscode.workspace
.getConfiguration("apollographql")
.get("devTools.serverPort", 0),
listening: !!serverState?.port,
},
});
}
}
},
Comment on lines +58 to +73
Copy link
Member Author

Choose a reason for hiding this comment

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

Where previously the panel kickstarted itself, it now notifies the extension, and the extension dispatches the initializePanel message with some additional context.

this,
panelDisposables,
);
function forwardToDevTools(data: unknown) {
panel.webview.postMessage(data);
}
devtoolsEvents.addListener("toDevTools", forwardToDevTools);
panel.onDidDispose(() => {
devtoolsEvents.removeListener("toDevTools", forwardToDevTools);
for (const disposable of panelDisposables) {
disposable.dispose();
}
});
}

Expand Down Expand Up @@ -107,11 +160,9 @@ export class DevToolsViewProvider implements vscode.WebviewViewProvider {
</script>
<script nonce="${nonce}" src="${scriptUri}"></script>
<script nonce="${nonce}">
window.originalPostMessage({
id: 123,
source: "apollo-client-devtools",
type: "actor",
message: { type: "initializePanel" },
window.postMessage({
source: "vscode-panel",
type: "mounted",
});
</script>
</body>
Expand Down
52 changes: 46 additions & 6 deletions src/devtools/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,53 @@ devtoolsEvents.addListener("fromDevTools", (msg) => {
Debug.info("DevTools > WS: " + JSON.stringify(msg));
});

export function startServer(port: number): Disposable {
let id = 1;

export function sendToDevTools(message: unknown) {
devtoolsEvents.emit("toDevTools", {
id: `vscode-${id++}`,
source: "apollo-client-devtools",
type: "actor",
message,
});
}

export let serverState:
| { port: false | number; disposable: Disposable }
| undefined = undefined;
Comment on lines +30 to +31
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why use false instead of null or undefined to represent the initial state?

Copy link
Member Author

Choose a reason for hiding this comment

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

It felt more deliberate than undefined. null would have been fine too, I guess.


export function startServer(port: number) {
Copy link
Member Author

Choose a reason for hiding this comment

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

A bunch of checks moved over from extension.ts here and the whole "disposing" and "restarting" should be a lot more solid now.

const state = {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

See a bit further down: if (serverState === state) { - it makes it possible to detect if serverState has been reassigned.

port: false as false | number,
disposable: new Disposable(() => {
if (wss) {
wss.close();
wss = null;
}
if (serverState === state) {
serverState = undefined;
}
sendToDevTools({ type: "port.changed", port, listening: false });
}),
};
if (serverState) {
if (serverState.port === port) {
// nothing to do
return;
} else {
// changing port, stop the old server
serverState.disposable.dispose();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Member

@jerelmiller jerelmiller Dec 6, 2024

Choose a reason for hiding this comment

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

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();
// ...

Copy link
Member Author

Choose a reason for hiding this comment

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

c902746

Copy link
Member Author

Choose a reason for hiding this comment

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

and d4a65ee ... that took a moment to click for me

serverState = state;
let wss: WebSocketServer | null = new WebSocketServer({ port });
wss.on("listening", () => {
state.port = port;
sendToDevTools({ type: "port.changed", port, listening: true });
});
wss.on("close", () => {
state.disposable.dispose();
});
runServer(wss, {
addListener: (listener) => {
devtoolsEvents.addListener("fromDevTools", listener);
Expand All @@ -28,9 +73,4 @@ export function startServer(port: number): Disposable {
devtoolsEvents.emit("toDevTools", message);
},
});

return new Disposable(() => {
wss?.close();
wss = null;
});
}
48 changes: 39 additions & 9 deletions src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import {
DecorationOptions,
commands,
QuickPickItem,
Disposable,
OutputChannel,
MarkdownString,
Range,
env,
} from "vscode";
import StatusBar from "./statusBar";
import { getLanguageServerClient } from "./languageServerClient";
Expand All @@ -27,8 +27,13 @@ import {
printStatsToClientOutputChannel,
} from "./utils";
import { Debug } from "./debug";
import { DevToolsViewProvider } from "./devtools/DevToolsViewProvider";
import { startServer } from "./devtools/server";
import {
DevToolsViewProvider,
isActorMessage,
isDevToolsExecuteCommandMessage,
isDevToolsOpenExternalMessage,
} from "./devtools/DevToolsViewProvider";
import { devtoolsEvents, serverState, startServer } from "./devtools/server";

const { version } = require("../package.json");

Expand Down Expand Up @@ -346,21 +351,46 @@ export async function activate(
context.subscriptions.push(
window.registerWebviewViewProvider(DevToolsViewProvider.viewType, provider),
);
let devtoolServer: Disposable | null = null;

function devToolsEventListener(event: unknown) {
if (!isActorMessage(event)) return;
const message = event.message;
if (isDevToolsExecuteCommandMessage(message)) {
commands.executeCommand(message.command, ...(message.arguments || []));
}
if (isDevToolsOpenExternalMessage(message)) {
env.openExternal(
// if we `Uri.parse` here, we end up with something that somehow double-encodes some things like `#`
// interestingly enough, the implementation of `openExternal` also allows for strings to be passed in
// directly, and that works - so we just pass in the string directly
message.uri as any as Uri,
);
}
}
devtoolsEvents.addListener("fromDevTools", devToolsEventListener);
context.subscriptions.push({
dispose: () =>
devtoolsEvents.removeListener("fromDevTools", devToolsEventListener),
});

context.subscriptions.push(
commands.registerCommand("apollographql/startDevToolsServer", () => {
const port = workspace
.getConfiguration("apollographql")
.get("devTools.serverPort", 0);
if (!devtoolServer && port) {
context.subscriptions.push((devtoolServer = startServer(port)));
}
startServer(port);
}),
);
context.subscriptions.push({
dispose() {
if (serverState) {
serverState.disposable.dispose();
}
},
});
context.subscriptions.push(
commands.registerCommand("apollographql/stopDevToolsServer", () => {
devtoolServer?.dispose();
devtoolServer = null;
serverState?.disposable.dispose();
}),
);

Expand Down