feature(dev-middleware): add custom message handlers to extend CDP capabilities

[byCedric]

Summary:

This is a proposal for the @react-native/dev-middleware package, to allow implementers to extend the CDP capabilities of the InspectorProxy. It's unfortunately needed until we can move to the native Hermes CDP layer.

At Expo, we extend the CDP capabilities of this InspectorProxy by injecting functionality on the device level. This proposed API does the same, but without having to overwrite internal functions of both the InspectorProxy and InspectorDevice.

A good example of this is the network inspector's capabilities. This currently works through the inspection proxy, and roughly like:

• Handle any incoming Expo(Network.receivedResponseBody) from the device, store it, and stop event from propagating
• Handle the incoming Network.getResponseBody from the debugger, return the data, and stop event from propagating.

This API brings back that capability in a more structured way.

API:

import { createDevMiddleware } from '@react-native/dev-middleware';

const { middleware, websocketEndpoints } = createDevMiddleware({
  unstable_customInspectorMessageHandler: ({ page, deviceInfo, debuggerInfo }) => {
    // Do not enable handler for page other than "SOMETHING", or for vscode debugging
    // Can also include `page.capabilities` to determine if handler is required
    if (page.title !== 'SOMETHING' || debuggerInfo.userAgent?.includes('vscode')) {
      return null;
    }

    return {
      handleDeviceMessage(message) {
        if (message.type === 'CDP_MESSAGE') {
          // Do something and stop message from propagating with return `true`
          return true;
        }
      },
      handleDebuggerMessage(message) {
        if (message.type === 'CDP_MESSAGE') {
          // Do something and stop message from propagating with return `true`
          return true;
        }
      },
    };
  },
});

Changelog:

[GENERAL] [ADDED] - Add inspector proxy device message middleware API

Test Plan:

See added tests and code above

[byCedric]

Edit: this is now applied to this PR

---

One other change we could make in this API, is to move "instantiation of middleware" to whenever the debugger is connecting to the device/page.

By doing that, we limit the middleware instantiation to exactly the logical pairs we need to intercept. And, when the debugger disconnects, we can "destroy" the middleware and empty states.

The API itself would look somewhat like this:

const { middleware, websocketEndpoints } = createDevMiddleware({
  unstable_deviceMessageMiddleware: ({ page, deviceInfo, debuggerInfo }) => {
    // Do not enable middleware for page other than "SOMETHING", or for vscode debugging
    // Can also include `page.capabilities` to determine if middleware is required
    if (page.title !== 'SOMETHING' || debuggerInfo.userAgent.includes('vscode')) {
      return null;
    }

    return {
      handleDeviceMessage(message) {
        if (message.type === 'CDP_MESSAGE') {
          // Do something
          return true;
        }
      },
      handleDebuggerMessage(message) {
        if (message.type === 'CDP_MESSAGE') {
          // Do something
          return true;
        }
      },
    };
  },
});

[motiz88]

This is a massive step in the right direction, thank you @byCedric! I have some thoughts about the specifics of the design. (I might do another pass for nits/naming after the design is more settled.)

[motiz88]

Using DebuggerInfo and Page as keys in a map seems brittle. Are there unique IDs we could use / generate for this purpose?

[motiz88]

Also, could we avoid WeakMap and structure our data differently to begin with, with proper cleanup when a connection is terminated etc?

[byCedric]

We could nest the middleware instance under the debugger connection instead. That way, once that is cleared, the middleware should be garbage collected as well. I think a debugger connection always is to a single Page/device, so that should match the scope of the middleware too.

But I probably need some help related to Flow typing on that one, in the previous iteration I put it on the Page object but ran into exact/in-exact object typing issues here

[byCedric]

✅ This is now nested inside the this.#debuggerConnection, and get's cleared once this connection is destroyed.

[motiz88]

1. Does handleDeviceMessage receive CDP messages or "inspector-proxy protocol" messages? It shouldn't be both.
2. Doesn't handleDeviceMessage return a value we need to branch on?

This looks like it "wants" to just be a separate disconnected() method on the middleware.

[byCedric]

I added a comment just above that on why the return value is not used here:

// NOTE(bycedric): Notify the device message middleware of the disconnect event, without any further actions.
// This can be used to clean up state in the device message middleware.

But, since we now scope the middleware on the actual device page and debugger connection, I don't think we need to do this anymore. The middleware should be fully garbage collected once either one disconnects. So we don't have to worry about disconnect events.

[byCedric]

✅ This is now removed, as the whole custom message handler is garbage collected once this happens.

[motiz88]

unsupported CDP events

Pedantic nit: It looks like we actually give the middleware precedence over passing messages to/from the app, rather than just passing "unsupported" events to it.

[motiz88]

per device

Per page, right?

[byCedric]

Pedantic nit: It looks like we actually give the middleware precedence over passing messages to/from the app, rather than just passing "unsupported" events to it.

Yes, unfortunately, we need to be able to influence "semi-supported" events too. It's mostly for vscode debugging, as some events are responded but not fully understood by vscode.js-debug. E.g. microsoft/vscode-js-debug#1583

There are more cases, or there could be more popping up. Having the ability to mutate or "replace"/workaround incompatible requests/responses would help us maintain the vscode debugging method. It's also the reason we implemented (and asked for) a debugger user agent, so we know if we need to enable more mutations etc.

Per page, right?

Your reaction is correct, I forgot to update this after the last iteration, will update.

[byCedric]

Per page, right?

✅ I updated the docs

[motiz88]

Does your use case not need the ability to also send notifications/events from the device to the debugger?

[byCedric]

Yes, both ways. I kept the Device name as this happens on device level, just like we would do with the future native Hermes CDP API. But open to other names ofc 😄

For now, I'll apply your naming suggestion to everything, so that would be CustomMessageHandler.

[byCedric]

✅ This is now fully renamed to CustomMessageHandler

[motiz88]

I don't think MessageFromDevice and MessageToDevice are the correct types here. Those represent the "inspector-proxy protocol", not CDP.

[byCedric]

Ah, my bad. It's a bit rough to find the right types here I think. Has to be generic as this library only types the supported events (while all events from both device and debugger are passing through these functions).

[byCedric]

⚠️ I don't think I used the right types, but since its a bit hard to strictly type this, I used the JSONSerializable type.

[motiz88]

We can't expose the raw socket to middleware. If the middleware needs the ability to send CDP messages, then CreateDeviceMessageMiddlewareFn should include a method to do that as a parameter.

[byCedric]

Makes sense, in our implementation I added the sendToDebugger and sendToDevice methods anyways. I think we can move them to the debuggerInfo/deviceInfo, without exposing the socket directly.

e.g.

• debuggerInfo.send = (message: DebuggerEvent | DebuggerMessage) => void
• deviceInfo.send = (message: DeviceEvent | DeviceMessage) => void

Would that solve your point here? Not 100% sure yet about the typing, but you get the point.

[byCedric]

I also thought about not allowing sending messages, and just return the message object from middleware (or null if it should stop propagating). But I think this would limit us too much in terms of supporting vscode.

[byCedric]

✅ I replaced the sockets with sendMessage methods.

Here are our custom handlers, and a quick check if this should work or not.

• NetworkResponse
  This sends a response once Network.getResponseBody is requested from the debugger, should work fine.
• PageReload
  Only replies with an acknowledge response to the debugger for Page.reload requests, should work fine.
• VscodeDebuggerGetPossibleBreakpoints
  Replies to the unsupported Debugger.getPossibleBreakpoints request, with an empty locations list (as vscode does not check for nullablility / the locations: [] also required in the CDP spec).
• VscodeDebuggerSetBreakpointByUrl
  Only mutates the message to avoid using unsupported way to set a breakpoint (through Debugger.setBreakpointByUrl)
• VscodeRuntimeCallFunctionOn
  Replies to the debugger to avoid vscode injecting unsupported web scripts inside Hermes, which crashes Hermes / app.
• VscodeRuntimeGetProperties
  Only mutates the message to avoid vscode performing actions on Symbols, which also crashes Hermes / app.

[motiz88]

Page.reload should check the nativePageReloads capability flags btw.

cc @dannysu for Hermes crashes, let's write Hermes unit tests / RN integration tests around them and see if they still occur in the new backend implementation

[motiz88]

LGTM, see minor comments. In general could be worth rethinking some of the naming for clarity, but given that this is a short-term unstable API, I'm OK with shipping this.

[motiz88]

Typically the thing to do is store the object in a local variable (const debuggerConnection = this.#debuggerConnection) and do the null checks on that. See https://flow.org/en/docs/lang/refinements/#toc-refinement-invalidations

[motiz88]

Naming nit: I don't think sendMessage() should be considered part of "info"

[byCedric]

✅ It's now just connection.device.sendMessage and connection.debugger.sendMessage.

[motiz88]

Why catch all exceptions here? Let's at minimum log them to debug()

[byCedric]

This is copied from the this.#sendMessageToDevice method:

react-native/packages/dev-middleware/src/inspector-proxy/Device.js

Lines 431 to 439 in cfa39c0

	// Sends single message to device.
	#sendMessageToDevice(message: MessageToDevice) {
	try {
	if (message.event !== 'getPages') {
	debug('(Debugger) (Proxy) -> (Device): ' + JSON.stringify(message));
	}
	this.#deviceSocket.send(JSON.stringify(message));
	} catch (error) {}
	}

Happy to add debugging logs for both though.

[motiz88]

Let's give this type a name (DebuggerConnection?)

[byCedric]

✅ Added and implemented the type

[motiz88]

If we create the DebuggerConnection type, we can probably use it here directly

[byCedric]

Yeah, we can definitely do that; I was hesitant about doing so as this method only needs DebuggerInfo.

[byCedric]

✅ Used DebuggerConnection instead

[byCedric]

Fix typo

[facebook-github-bot]

@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[github-actions]

This pull request was successfully merged by @byCedric in 3f41fb5.

^{When will my fix make it into a release? | Upcoming Releases}