DevTools <-> VSCode integration by phryneas · Pull Request #1497 · apollographql/apollo-client-devtools

phryneas · 2024-08-29T16:42:27Z

This is in very early stages and split up between here and apollographql/vscode-graphql#188

phryneas · 2024-08-30T11:40:11Z

And the Explorer tab is working 🎉

phryneas · 2024-09-04T14:41:13Z

I'll fork off here into additional features - this is a good size for a review already :)

jerelmiller

I might have more with fresh eyes, but I think this is a good set of feedback to take a look at for now. I'd definitely recommend seeing if you can reduce to a single rpc client/message adapter in the server if possible so you can take advantage of the message bridge.

jerelmiller · 2024-09-13T06:17:41Z

@@ -0,0 +1,36 @@
+{
+  "name": "@apollo/client-devtools-vscode",
+  "version": "4.18.4-pre.0",


Should this match the initial version in package.json? Looks like that is on 4.18.6 if you want to update the patch version here to keep it in sync (though not sure how much it matters yet at this stage or not)

That was just for my local build back when I forked off - when we publish the package, it will go through changesets and inherit the version of the extension :)

jerelmiller · 2024-09-16T06:24:31Z

+  };
+}
+
+function createWsAdapter(ws: WebSocket, signal: AbortSignal): MessageAdapter {


I think it would be good to do a bit of organization on the existing code so that this is near the other code, but ok for now and we can refactor in a future PR.

jerelmiller · 2024-09-16T06:34:11Z

+    connected: new Promise<void>((resolve) =>
+      ws.addEventListener("open", () => resolve(), { once: true, signal })
+    ),


I'm not a huge fan of this name mostly because reading this I keep expecting this to be a boolean, but its a promise instead.

Out of curiosity, could we just use a callback here and call this onConnected? We can remove the promise altogether then.

Suggested change

connected: new Promise<void>((resolve) =>

ws.addEventListener("open", () => resolve(), { once: true, signal })

),

onConnected: (callback: () => void) => {

ws.addEventListener("open", () => callback(), { once: true, signal })

},

That would miss the event if you added an onConnected event after the connection is already established and handling that would add extra logic. I'm open to changing the name, but I'd like to keep the promise, as that handles that nicely.

Thats a good point. Let's see if we can rename this then.

Would it be out of the question for registerClient (or whatever the new name will be) to return a promise instead? If not, my brain is out of names 😆

connectedPromise - will at least not be confused with a boolean, and also rejects now when there is a connection error

jerelmiller · 2024-09-16T06:37:31Z

+    unregister: cleanup,
+    onCleanup: registerCleanup,


Suggested change

unregister: cleanup,

onCleanup: registerCleanup,

disconnect: cleanup,

onDisconnect: registerCleanup,

What about calling this disconnect? I think this will probably be a bit more intuitive than "cleanup" since its not immediately clear what is being cleaned up without knowing the implementation details. Thoughts?

It's called unregister right now, because the function itself is named registerClient.

We could rename registerClient to connectClientToDevTools or something, then disconnect would make sense 🤔
Not sure if I like that better, though - WDYT?

Actually I do like that better! I had a similar thought this morning before reading your comment so I'm on board. A name like connectClienttoDevTools makes it immediately obvious what that block of code does without having to follow the import to understand what you're registering your client with. On top of that, it better follows with our connectToDevTools option naming we've had in Apollo Client for for a while so theres some symmetry there 🙂

Then I'd suggest we make it really obvious with a connectApolloClientToVSCodeDevTools? :)

Haha either one, but something with connect and devTools in it makes sense to me :)

renamed to connectApolloClientToVSCodeDevTools and also edited the shape of the returned object

jerelmiller · 2024-09-16T06:46:13Z

+    const wsRpcHandler = createRpcHandler(wsAdapter);
+    const wsActor = createActor(wsAdapter);
+    wsActor.on("registerClient", ({ payload }) => {
+      payload.id = id;


Ah ok I see this is set here instead of in client.ts, which I missed when adding the other comment.

I'm curious why you decided to go this route of creating a handler/rpc client per apollo client instance? If you're able to use a single client/rpc handler, you would then be able to take advantage of the message bridge since this script would essentially just be the middle man between the client script and devtools.

I'd recommend taking a stab at that, otherwise we'll need to edit this file anytime we want to add a new message from devtools to the client (such as the memory panel that will be added soon). I used to do this in tab.ts and it there were definitely times that I scratched my head wondering why something wasn't working only to realize I forgot to add the message forwarding between the two. The message bridge definitely helps out since its a bit more automatic.

The message bridge would work if we had the guarantee of a single connection, but you can totally connect two different processes to one server (e.g. connect an iOS emulator and an Android emulator at the same time). So either at that point I send everything everywhere, or I have to keep track of who is who in the server - I went for the latter.

Also, we could in the future add some "snapshot" functionality here inside the server that would work by creating a "virtual client" that would be fully controlled by the server.

Ah right that makes sense.

I suppose the thing that worries me is diverging some of the client-specific behavior too much from each other between the browser extension and the vscode integration. It makes sense that we have different transport mechanisms (websockets vs ports) and have varying code there, but if we can make some of the core client logic the same, I think it will be easier to maintain long-term.

In this case, I'm specifically referring to where/how the client id is created and assigned. In the browser extension, this is created when registering the client with the browser extension. Here it is created and assigned in the server instead. If we end up diverging too much on this kind of thing, I fear it will get difficult to maintain as it means some of the common abstractions may only work in one of two contexts.

I'm not opposed to assigning it in the server, but if we go that route, I'd love for the browser extension to resemble this structure as well. It wasn't immediately obvious to me id was going to be assigned outside of registerClient in the code when reading through this the first time (hence my barrage of comments).

For now though, what do you think about at least creating the id over in registerClient and just reading that here instead of creating/assigning it in the server?

Creating the id inregisterClient now, I just hope there won't be any collisions in low-entropy environments

I don't know that I'm that worried to be honest. At most we'll have maybe 10s of clients connected at once, at least in the 99% of cases. If we get reports of collisions between connected clients because we start getting tons of them, we can always revisit.

* change option names * add docblocks * pass `signal` around further instead of registering cleanup callbacks

jerelmiller

This looks great! Thanks for making those changes 💯

jerelmiller · 2024-09-18T05:15:34Z

+    const wsRpcHandler = createRpcHandler(wsAdapter);
+    const wsActor = createActor(wsAdapter);
+    wsActor.on("registerClient", ({ payload }) => {
+      payload.id = id;


I don't know that I'm that worried to be honest. At most we'll have maybe 10s of clients connected at once, at least in the 99% of cases. If we get reports of collisions between connected clients because we start getting tons of them, we can always revisit.

phryneas mentioned this pull request Aug 29, 2024

DevTools <-> VSCode integration apollographql/vscode-graphql#188

Merged

phryneas marked this pull request as ready for review September 4, 2024 14:40

phryneas requested a review from a team as a code owner September 4, 2024 14:40

phryneas added 12 commits September 10, 2024 09:54

update @apollo/tailwind-preset to ^0.2.0

9a5e81e

set base font size

8e24aa9

font size in :root

21d2dde

first working version

97f1371

add explorer support to vscode

06eb543

build @apollo/client-devtools-vscode

b7f47df

add cleaup functionality

8ba0fd2

also expose onCleanup

414c02e

Work around maximum listener count in node

6750e31

do not hold onto client and unregister once client leaves memory

26ffdb2

ignore yalc

8b43491

track reason for cleanup

b5c0ee6

phryneas force-pushed the pr/vscode branch from 0e78e10 to b5c0ee6 Compare September 10, 2024 08:14

jerelmiller changed the title ~~[WIP] DevTools <-> VSCode integration~~ DevTools <-> VSCode integration Sep 12, 2024

jerelmiller requested changes Sep 16, 2024

View reviewed changes

phryneas added 5 commits September 16, 2024 16:35

url -> vsCodeServerUrl

b19403d

adjust peerDependency version

d9cf880

rename tab to vscodeActor, create id in the client

5d86a96

rename registerClient to connectApolloClientToVSCodeDevTools

5bc7aa4

* change option names * add docblocks * pass `signal` around further instead of registering cleanup callbacks

semgrepignore

1b40aa0

jerelmiller approved these changes Sep 18, 2024

View reviewed changes

phryneas added 2 commits September 20, 2024 14:25

Merge remote-tracking branch 'origin/main' into pr/vscode

af28b42

move ws to devDependencies

b457e91

phryneas merged commit 9aabb7a into main Sep 20, 2024

phryneas deleted the pr/vscode branch September 20, 2024 12:30

Conversation

phryneas commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phryneas commented Aug 30, 2024

Uh oh!

phryneas commented Sep 4, 2024

Uh oh!

jerelmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phryneas Sep 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerelmiller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

phryneas commented Aug 29, 2024 •

edited

Loading

phryneas Sep 16, 2024 •

edited

Loading