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.

Sign up for GitHub

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

Allow client-side device identifiers in inspector proxy #991

Closed
wants to merge 6 commits into from
Closed

Allow client-side device identifiers in inspector proxy #991

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c52f355
Allow string device ids in inspector proxy
byCedric May 27, 2023
f2ca178
Allow device identifiers when registering inspectable device
byCedric May 27, 2023
8c4a827
Fix some linting issue
byCedric May 27, 2023
9bbf992
Move debugger socket reuse logic to device
byCedric Jun 2, 2023
e4885b1
Update new device connection debug log
byCedric Jun 2, 2023
1d516da
Use docblock instead of line comments
byCedric Jun 2, 2023
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
2 changes: 1 addition & 1 deletion packages/metro-inspector-proxy/src/Device.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const REACT_NATIVE_RELOADABLE_PAGE_ID = '-1';
*/
class Device {
// ID of the device.
_id: number;
_id: string;

// Name of the device.
_name: string;
Expand Down
24 changes: 19 additions & 5 deletions packages/metro-inspector-proxy/src/InspectorProxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class InspectorProxy {
_projectRoot: string;

// Maps device ID to Device instance.
_devices: Map<number, Device>;
_devices: Map<string, Device>;

// Internal counter for device IDs -- just gets incremented for each new device.
_deviceCounter: number = 0;
Expand Down Expand Up @@ -111,7 +111,7 @@ class InspectorProxy {
// Converts page information received from device into PageDescription object
// that is sent to debugger.
_buildPageDescription(
deviceId: number,
deviceId: string,
device: Device,
page: Page,
): PageDescription {
Expand Down Expand Up @@ -162,16 +162,30 @@ class InspectorProxy {
// $FlowFixMe[value-as-type]
wss.on('connection', async (socket: WS, req) => {
try {
const fallbackDeviceId = String(this._deviceCounter++);

const query = url.parse(req.url || '', true).query || {};
const deviceId = query.device || fallbackDeviceId;
const deviceName = query.name || 'Unknown';
const appName = query.app || 'Unknown';
const deviceId = this._deviceCounter++;

const oldDevice = this._devices.get(deviceId);
if (oldDevice) {
// Keep the debugger connection alive when disconnecting the device, if possible
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the new Device created below somehow inherit _debuggerConnection from oldDevice?

Also, _debuggerConnection is a private variable (by naming convention) and ideally shouldn't be accessed outside of the Device class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the new Device created below somehow inherit _debuggerConnection from oldDevice?

It does, but we might need to add some logic when swapping out the Device instances. Basically, in my tests I found that the open debugger automatically triggered the Device.handleDebuggerConnection method (background reconnection). This sets the debugger connection to the new Device instance.

I think an ideal scenario is to just invoke that method directly during the initialization.

Originally, I had to unset the debugger connection in order to avoid the device's socket close event handler.

Copy link
Contributor Author

@byCedric byCedric Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, _debuggerConnection is a private variable (by naming convention) and ideally shouldn't be accessed outside of the Device class

One thing we can do is move the shutdown logic into a handler on the device class. With that, we don't need to access _debuggerConnection, _name, and _app outside the class. Does that make sense to you? @voideanvalue

Copy link
Member

@huntie huntie Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yup makes sense. I think feel free to modify the Device class (as it's private within this package) to accomplish the new logic needed in a well-named method(s).

if (oldDevice._name === deviceName && oldDevice._app === appName) {
oldDevice._debuggerConnection = null;
}
oldDevice._deviceSocket.close();
}

this._devices.set(
deviceId,
new Device(deviceId, deviceName, appName, socket, this._projectRoot),
);

debug(`Got new connection: device=${deviceName}, app=${appName}`);
debug(
`Got new connection: device=${deviceName}, app=${appName}, id=${deviceId}`,
);

socket.on('close', () => {
this._devices.delete(deviceId);
Expand Down Expand Up @@ -206,7 +220,7 @@ class InspectorProxy {
throw new Error('Incorrect URL - must provide device and page IDs');
}

const device = this._devices.get(parseInt(deviceId, 10));
const device = this._devices.get(deviceId);
if (device == null) {
throw new Error('Unknown device with ID ' + deviceId);
}
Expand Down