Skip to content

Commit

Permalink
Fix bug in device ID collision handling, add tests (#45010)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #45010

D46482492 added logic for handing off state across "device" connections that have the same ID. This logic currently has no test coverage. It also contains a bug whereby the new device's pages are removed from the target listing endpoint (`/json`) when the *old* device's socket is closed.

This diff adds tests and fixes the bug.

Changelog: [General][Fixed] inspector-proxy no longer accidentally detaches connected devices.

## Next steps

It seems that the device ID handoff logic exists to paper over a deeper problem with the inspector proxy protocol (or its implementation in React Native): The React Native runtime should not routinely be creating new "device" connections without tearing down previous ones.

In followup diffs, I'll explore changing this behaviour for Fusebox, based on the new test coverage.

Reviewed By: robhogan

Differential Revision: D51013056

fbshipit-source-id: e0c17678cc747366a3b75cef18ca2a722fc93acd
  • Loading branch information
motiz88 authored and facebook-github-bot committed Jun 17, 2024
1 parent 9491ded commit 4c6bff0
Show file tree
Hide file tree
Showing 5 changed files with 360 additions and 4 deletions.
6 changes: 6 additions & 0 deletions packages/dev-middleware/src/__tests__/InspectorDeviceUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import type {
WrappedEvent,
} from '../inspector-proxy/types';

import nullthrows from 'nullthrows';
import WebSocket from 'ws';

export class DeviceAgent {
Expand Down Expand Up @@ -82,6 +83,11 @@ export class DeviceAgent {
},
});
}

// $FlowIgnore[unsafe-getters-setters]
get socket(): WebSocket {
return nullthrows(this.#ws);
}
}

export class DeviceMock extends DeviceAgent {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,15 @@ export async function createAndConnectTarget(
}>,
signal: AbortSignal,
page: PageFromDevice,
deviceId: ?string = null,
): Promise<{device: DeviceMock, debugger_: DebuggerMock}> {
let device;
let debugger_;
try {
device = await createDeviceMock(
`${serverRef.serverBaseWsUrl}/inspector/device?device=device&name=foo&app=bar`,
`${serverRef.serverBaseWsUrl}/inspector/device?device=${
deviceId ?? 'device' + Date.now()
}&name=foo&app=bar`,
signal,
);
device.getPages.mockImplementation(() => [page]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@
* @oncall react_native
*/

import type {PageDescription} from '../inspector-proxy/types';
import type {
JsonPagesListResponse,
PageDescription,
} from '../inspector-proxy/types';

import {fetchJson} from './FetchUtils';
import {createDebuggerMock} from './InspectorDebuggerUtils';
Expand Down Expand Up @@ -51,7 +54,7 @@ describe.each(['HTTP', 'HTTPS'])(
},
]);

let pageList: Array<PageDescription> = [];
let pageList: JsonPagesListResponse = [];
await until(async () => {
pageList = (await fetchJson(
`${serverRef.serverBaseUrl}/json`,
Expand Down
Loading

0 comments on commit 4c6bff0

Please sign in to comment.