refactor: inspect/debugging as `useInspector` #199

threepointone · 2022-01-06T23:38:33Z

This PR rewrites the logic for establishing a connection with the debugging socket, and adds some enhancements along the way. Part of solving #188. Some highlights -

I've installed devtools-protocol, a convenient package that has the static types for the devtools protocol (duh) autogenerated from chrome's devtools codebase.
We now log messages and exceptions into the terminal directly, so you don't have to open devtools to see those messages.
Messages are now buffered until a devtools instance connects, so you won't lose any messages while devtools isn't connected.
We don't lose the connection on making changes to the worker, removing the need for the kludgy hack on the devtools side (where we refresh the whole page when there's a change)

Some things that I still have to do in followup PRs -

clear the console whenever we make a change to the worker
stay connected when we shift between local/remote mode

changeset-bot · 2022-01-06T23:38:36Z

🦋 Changeset detected

Latest commit: e3fa749

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/wrangler/src/inspect.ts

threepointone · 2022-01-07T14:41:13Z

packages/wrangler/src/dev.tsx

-  useInspector(token ? token.inspectorUrl.href : undefined);
+  useInspector({
+    inspectorUrl: token ? token.inspectorUrl.href : undefined,
+    port: 9229,


in a followup PR, we'll make this port configurable

And/or also if not configured, dynamic to avoid an already open port?

packages/wrangler/src/inspect.ts

petebacondarwin

I love the use of hooks here to keep dependencies in sync. Very nice!

I made a bunch of comments, but I don't think any are blocking this PR, assuming that you have actually checked it works as expected!

Can you provide some thoughts on how you propose we write tests for all this functionality?

petebacondarwin · 2022-01-07T14:34:51Z

packages/wrangler/src/dev.tsx

@@ -196,7 +162,11 @@ function Remote(props: {

  useProxy({ token, publicRoot: props.public, port: props.port });

-  useInspector(token ? token.inspectorUrl.href : undefined);
+  useInspector({
+    inspectorUrl: token ? token.inspectorUrl.href : undefined,


NIT:

Suggested change

inspectorUrl: token ? token.inspectorUrl.href : undefined,

inspectorUrl: token?.inspectorUrl.href,

petebacondarwin · 2022-01-07T14:40:58Z

packages/wrangler/src/inspect.ts

+ * When we start a session with `wrangler dev`, the Workers platform
+ * also exposes a debugging websocket that implements the DevTools
+ * Protocol. While we could just start up DevTools and connect to this
+ * URL, however that URL changes every time we make a change to the


Suggested change

* URL, however that URL changes every time we make a change to the

* URL, that URL changes every time we make a change to the

petebacondarwin · 2022-01-08T14:27:02Z

packages/wrangler/src/dev.tsx

-  useInspector(token ? token.inspectorUrl.href : undefined);
+  useInspector({
+    inspectorUrl: token ? token.inspectorUrl.href : undefined,
+    port: 9229,


And/or also if not configured, dynamic to avoid an already open port?

petebacondarwin · 2022-01-08T14:35:15Z

packages/wrangler/src/inspect.ts

+    // Let's create the websocket server on top of the proxy server
+    wsServerRef.current = new WebSocketServer({ server: serverRef.current });
+    wsServerRef.current.on("connection", (ws: WebSocket) => {
+      if (isConnected) {


Could you do something like the following?

if (wsServerRef.current.clients.size > 1) {

nice. will do.

petebacondarwin · 2022-01-08T14:41:52Z

packages/wrangler/src/inspect.ts

+     */
+    let isConnected = false;
+    // Let's create the websocket server on top of the proxy server
+    wsServerRef.current = new WebSocketServer({ server: serverRef.current });


According to this https://github.com/websockets/ws/blob/HEAD/doc/ws.md#serverclients we need to set clientTracking: true in these options so that the clients property used below is active.

Huh, I could swear I tested this. Thanks for the catch!

petebacondarwin · 2022-01-08T15:02:15Z

packages/wrangler/src/inspect.ts

+      messageBufferRef.current.forEach((evt) => {
+        localWebSocket.send(evt);
+      });


And if we renamed remoteMessageListener to sendMessageToLocal then it would have been more obvious to reuse that here:

Suggested change

messageBufferRef.current.forEach((evt) => {

localWebSocket.send(evt);

});

messageBufferRef.current.forEach(sendMessageToLocal);

petebacondarwin · 2022-01-08T15:06:32Z

packages/wrangler/src/inspect.ts

+
+/**
+ * This function converts a message serialised as a devtools event
+ * into aarguments uitable to be called by a console method, and


Suggested change

* into aarguments uitable to be called by a console method, and

* into arguments suitable to be called by a console method, and

petebacondarwin · 2022-01-08T15:09:43Z

packages/wrangler/src/inspect.ts

+    }
+  }
+
+  console[evt.type](...args);


Given that this is running in ink, I wonder if we could pass these messages out to be rendered in an Ink component, rather than just dumping them out using console.xxx?
(I appreciate that Ink patches console but I think we could be more elegant here...

We can revisit it. I actually try to avoid Ink for anything that doesn't feel like UI (and before you say it, I understand the boudnary is deeply fuzzy.) For example, in this usecase, I want the messages to persist no matter what, even if the process exits, etc. For now I think calling console methods makes sense, and matches behaviour closer to node (or at least, is already better than what we have, which is nothing.)

petebacondarwin · 2022-01-08T15:10:13Z

packages/wrangler/src/inspect.ts

+    wsServerRef.current = new WebSocketServer({ server: serverRef.current });
+    wsServerRef.current.on("connection", (ws: WebSocket) => {
+      if (isConnected) {
+        console.log(


console.warn or console.error here?

petebacondarwin · 2022-01-08T15:17:13Z

packages/wrangler/src/inspect.ts

+      }, 10_000);
+    });
+
+    ws.on("unexpected-response", () => {


Is this just something that you have noticed or is it documented somewhere?

Noticed! It actually happens. Tho, good point, it's not in the spec. I wonder whether we do anything special here for this message. I'll investigate.

threepointone

I updated the PR with suggested changes. I'm going to write down some followup tasks, including testing plan, before I land this. Thanks @petebacondarwin!

threepointone · 2022-01-11T09:15:19Z

packages/wrangler/src/inspect.ts

+      }, 10_000);
+    });
+
+    ws.on("unexpected-response", () => {


Noticed! It actually happens. Tho, good point, it's not in the spec. I wonder whether we do anything special here for this message. I'll investigate.

threepointone · 2022-01-11T09:16:29Z

packages/wrangler/src/inspect.ts

+          // TODO: maybe we should have a max limit on this?
+          // if so, we should be careful when removing messages
+          // from the front, because they could be critical for
+          // devtools (like execution context creation, etc)


Actually in my tests, there don't seem to be any critical messages per se. But I could be mistaken. Let's revisit this when it becomes a problem.

threepointone · 2022-01-11T13:06:35Z

packages/wrangler/src/inspect.ts

+     */
+    let isConnected = false;
+    // Let's create the websocket server on top of the proxy server
+    wsServerRef.current = new WebSocketServer({ server: serverRef.current });


Huh, I could swear I tested this. Thanks for the catch!

threepointone · 2022-01-11T13:07:31Z

packages/wrangler/src/inspect.ts

+    // Let's create the websocket server on top of the proxy server
+    wsServerRef.current = new WebSocketServer({ server: serverRef.current });
+    wsServerRef.current.on("connection", (ws: WebSocket) => {
+      if (isConnected) {


nice. will do.

threepointone · 2022-01-11T13:10:56Z

packages/wrangler/src/inspect.ts

+   * of the effect that connects to the remote websocket.
+   */
+  const [
+    retryRemoteWebSocketConnectionSigil,


I want a good word for something that's used to trigger the effect again. A sigil is used through history as a sign/seal of something. I would say counter, but that's the implementation detail. Got any suggestions for something better?

threepointone · 2022-01-11T13:12:55Z

packages/wrangler/src/inspect.ts

+      try {
+        remoteWebSocket.send(event.data);
+      } catch (e) {
+        if (e.message !== "WebSocket is not open: readyState 0 (CONNECTING)") {


The message is brittle. I highly dount it's part of the spec. Honestly maybe this can just be silent. When I add a logger for #230 I'll revisit this. Leaving this conversation unresolved so I can find it later.

threepointone · 2022-01-11T13:17:09Z

packages/wrangler/src/inspect.ts

+    }
+  }
+
+  console[evt.type](...args);


We can revisit it. I actually try to avoid Ink for anything that doesn't feel like UI (and before you say it, I understand the boudnary is deeply fuzzy.) For example, in this usecase, I want the messages to persist no matter what, even if the process exits, etc. For now I think calling console methods makes sense, and matches behaviour closer to node (or at least, is already better than what we have, which is nothing.)

This PR rewrites the logic for establishing a connection with the debugging socket, and adds some enhancements along the way. Part of solving #188. Some highlights - - I've installed `"devtools-protocol`, a convenient package that has the static types for the devtools protocol (duh) autogenerated from chrome's devtools codebase. - We now log messages and exceptions into the terminal directly, so you don't have to open devtools to see those messages. - Messages are now buffered until a devtools instance connects, so you won't lose any messages while devtools isn't connected. - We don't lose the connection on making changes to the worker, removing the need for the kludgy hack on the devtools side (where we refresh the whole page when there's a change) Some things that I still have to do, and will do so in followup PRs - - clear the console whenever we make a change to the worker - stay connected when we shift between local/remote mode - eventually, move to using the devtools hosted at cloudflareworkers.com.

threepointone requested a review from petebacondarwin January 6, 2022 23:38

threepointone commented Jan 6, 2022

View reviewed changes

packages/wrangler/src/inspect.ts Show resolved Hide resolved

threepointone force-pushed the fix-inspector branch from a5f787f to 16a1298 Compare January 6, 2022 23:42

threepointone commented Jan 7, 2022

View reviewed changes

packages/wrangler/src/inspect.ts Outdated Show resolved Hide resolved

Electroid reviewed Jan 7, 2022

View reviewed changes

packages/wrangler/src/inspect.ts Show resolved Hide resolved

threepointone commented Jan 7, 2022

View reviewed changes

JacobMGEvans reviewed Jan 7, 2022

View reviewed changes

packages/wrangler/src/inspect.ts Show resolved Hide resolved

threepointone force-pushed the fix-inspector branch from 16a1298 to a2b5f16 Compare January 7, 2022 22:27

petebacondarwin approved these changes Jan 8, 2022

View reviewed changes

petebacondarwin added the cleanup label Jan 8, 2022

threepointone force-pushed the fix-inspector branch from a2b5f16 to 16ff859 Compare January 11, 2022 13:57

threepointone commented Jan 11, 2022

View reviewed changes

threepointone force-pushed the fix-inspector branch from 16ff859 to 0f0dea4 Compare January 11, 2022 14:05

threepointone force-pushed the fix-inspector branch from 0f0dea4 to e3fa749 Compare January 11, 2022 15:22

threepointone merged commit d9ecb70 into main Jan 11, 2022

threepointone deleted the fix-inspector branch January 11, 2022 16:05

github-actions bot mentioned this pull request Jan 11, 2022

Version Packages #206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: inspect/debugging as `useInspector` #199

refactor: inspect/debugging as `useInspector` #199

threepointone commented Jan 6, 2022

changeset-bot bot commented Jan 6, 2022 •

edited

Loading

threepointone Jan 7, 2022

petebacondarwin Jan 8, 2022

petebacondarwin left a comment

petebacondarwin Jan 7, 2022

petebacondarwin Jan 7, 2022

petebacondarwin Jan 8, 2022

petebacondarwin Jan 8, 2022

threepointone Jan 11, 2022

petebacondarwin Jan 8, 2022

threepointone Jan 11, 2022

petebacondarwin Jan 8, 2022

petebacondarwin Jan 8, 2022

petebacondarwin Jan 8, 2022

threepointone Jan 11, 2022

petebacondarwin Jan 8, 2022

petebacondarwin Jan 8, 2022

threepointone Jan 11, 2022

threepointone left a comment

threepointone Jan 11, 2022

threepointone Jan 11, 2022

threepointone Jan 11, 2022

threepointone Jan 11, 2022

threepointone Jan 11, 2022

threepointone Jan 11, 2022

threepointone Jan 11, 2022

	inspectorUrl: token ? token.inspectorUrl.href : undefined,
	inspectorUrl: token?.inspectorUrl.href,

	* URL, however that URL changes every time we make a change to the
	* URL, that URL changes every time we make a change to the

	* into aarguments uitable to be called by a console method, and
	* into arguments suitable to be called by a console method, and

refactor: inspect/debugging as useInspector #199

refactor: inspect/debugging as useInspector #199

Conversation

threepointone commented Jan 6, 2022

changeset-bot bot commented Jan 6, 2022 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threepointone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor: inspect/debugging as `useInspector` #199

refactor: inspect/debugging as `useInspector` #199

changeset-bot bot commented Jan 6, 2022 •

edited

Loading