Allow client-side device identifiers in inspector proxy #991

byCedric · 2023-05-27T20:04:48Z

Summary

Fixes #985

This allows devices connecting to the inspector proxy to provide a client-side unique string identifier. With this, we can pass along a unique identifier that doesn't change when reconnecting, e.g. after a hard crash.

It makes the overall debugging experience way more stable since you don't have to restart the debugger in between crashes or full restarts. Allowing users to keep the debugger open also lets the debugger "remember" certain things, such as set breakpoints.

⚠️ When the client doesn't specify this unique identifier, it still falls back to the incremental identifier (the fallbackDeviceId in this PR)

When a collision occurs, the old device's connection is closed. But, if both the device and app names are equal to the new device connection, the debugger connection is kept open.

Next steps

If this is landed, we still need changes in React Native (the &device=... query param). I have no strong opinions on what identifier is used, but it should follow these rules:

Should be unique per device or emulator/simulator (and thus, also per platform)
Must not change when restarting the app
Must be a string that's URL safe

On Android, I had good success using the Secure.ANDROID_ID. It may change after a factory reset, which doesn't matter for our use case.

Test plan

Create a new project, and enable building from source on both Android & iOS
android: edit this file in node_modules/react-native and add a hardcoded &device=testingandroid query param.
ios: edit this file in node_modules/react-naive and add a hardcoded &device=testingios query param.
Connect the debugger to the running app
Force close the app, which should cause a "reconnect" warning in the debugger
Open the app again, and press "reconnect" in the debugger
Due to the stable identifiers, the URL won't change and the above scenario should work fine

Also test without these &device=... query param since that should work as it does now (with incremental identifiers).

voideanvalue · 2023-05-30T10:37:57Z

packages/metro-inspector-proxy/src/InspectorProxy.js

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


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

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.

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

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).

packages/metro-inspector-proxy/src/Device.js

packages/metro-inspector-proxy/src/InspectorProxy.js

huntie · 2023-06-02T11:45:02Z

Thanks Cedric! LGTM with small nits 🙂. Will import and it'll receive a final review.

The criteria for a device/session ID sound good — a stable identifier that is unique per device and app. If you need, I'm happy to handle the next steps on the React Native side.

Co-authored-by: Alex Hunt <[email protected]>

byCedric · 2023-06-02T14:25:07Z

If you need, I'm happy to handle the next steps on the React Native side.

That would be great 🙏

motiz88 · 2023-06-05T12:12:29Z

The criteria for a device/session ID sound good — a stable identifier that is unique per device and app.

To nitpick slightly - a device ID and session ID don't sound like the same thing. It's important to consider what happens if we have multiple devices, multiple apps on each device, and possibly even multiple instances of React Native in each app (and in the future, multiple JavaScript contexts e.g. Web Workers in each instance of React Native) - all of which could reasonably be connected to one instance of Metro.

This PR looks good, but eventually we should figure out all the different entities and their lifetimes, and flesh out what types of IDs (and persistence/stability) we need at each level.

facebook-github-bot · 2023-06-06T15:05:09Z

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

byCedric · 2023-06-07T12:53:46Z

@motiz88 fully agree, might also be good to check with Hermes if we need to reinitialize certain events. E.g. does Hermes enable debugging only based on the Debugger.enable event?

With their input, I'm sure we can come up with a great overview of each component, its life cycle, and what happens when non-happy-path things occur.

facebook-github-bot · 2023-06-07T13:07:07Z

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

facebook-github-bot · 2023-06-08T09:23:59Z

@huntie merged this pull request in c6a94bc.

… when restarting app (#22742) # Why After using the debugger for some time, I noticed a really annoying behavior when the app crashes or is restarted manually. See facebook/metro#985 for more info. # How This adds support for client-side unique device identifiers, such as "installation ids" or "device ids". Similarly to future support within Metro: facebook/metro#991 # Test Plan See updated tests, and the test plan at facebook/metro#991 > - Create a new project, and enable building from source on both Android & iOS > - **android**: edit [this file](https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevServerHelper.java#L282-L289) in `node_modules/react-native` and add a hardcoded `&device=testingandroid` query param. > - **ios**: edit [this file](https://github.com/facebook/react-native/blob/main/packages/react-native/React/DevSupport/RCTInspectorDevServerHelper.mm#L43-L53) in `node_modules/react-naive` and add a hardcoded `&device=testingios` query param. > - Connect the debugger to the running app > - Force close the app, which should cause a "reconnect" warning in the debugger > - Open the app again, and press "reconnect" in the debugger > - _Due to the stable identifiers, the URL won't change and the above scenario should work fine_ # Checklist  - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin).

Summary: Building on byCedric's approach in facebook/metro#991, adds support for passing a `device=...` argument to `/open-debugger` for more precise targeting. Changelog: [Internal] --- ## Note on what "device" means in this context In `dev-middleware` / `inspector-proxy`, "device" is something of a misnomer. It refers to a *logical device* containing one or more *pages*. In React Native, each app process forms its own logical device in which individual VMs register themselves as pages. An instance of `inspector-proxy` connects one or more *debuggers* (frontends) to one or more logical devices (one frontend to one page on one device). The intent of the logical device ID is to help with target discovery and especially *re*discovery - to reduce the number of times users need to explicitly close and restart the debugger frontend (e.g. after an app crash). If provided, the logical device ID: 1. SHOULD be stable for the current combination of physical device (or emulator instance) and app. 2. SHOULD be stable across installs/launches of the same app on the same device (or emulator instance), though it MAY be user-resettable (so as to not require any special privacy permissions). 3. MUST be unique across different apps on the same physical device (or emulator). 4. MUST be unique across physical devices (or emulators). 5. MUST be unique for each concurrent *instance* of the same app on the same physical device (or emulator). NOTE: The uniqueness requirements are stronger (MUST) than the stability requirements (SHOULD). In particular, on platforms that allow multiple instances of the same app to run concurrently, requirements 1 and/or 2 MAY be violated in order to meet requirement 5. This will be relevant, for example, on desktop platforms. In an upcoming diff, we will pass device IDs meeting these criteria from both iOS and Android. Reviewed By: huntie, blakef Differential Revision: D49954920 fbshipit-source-id: 4028e717f2f0ab1fc00f6db0ad154a85d8361c17

Summary: Building on byCedric's approach in facebook/metro#991, adds support for passing a `device=...` argument to `/open-debugger` for more precise targeting. Changelog: [Internal] --- ## Note on what "device" means in this context In `dev-middleware` / `inspector-proxy`, "device" is something of a misnomer. It refers to a *logical device* containing one or more *pages*. In React Native, each app process forms its own logical device in which individual VMs register themselves as pages. An instance of `inspector-proxy` connects one or more *debuggers* (frontends) to one or more logical devices (one frontend to one page on one device). The intent of the logical device ID is to help with target discovery and especially *re*discovery - to reduce the number of times users need to explicitly close and restart the debugger frontend (e.g. after an app crash). If provided, the logical device ID: 1. SHOULD be stable for the current combination of physical device (or emulator instance) and app. 2. SHOULD be stable across installs/launches of the same app on the same device (or emulator instance), though it MAY be user-resettable (so as to not require any special privacy permissions). 3. MUST be unique across different apps on the same physical device (or emulator). 4. MUST be unique across physical devices (or emulators). 5. MUST be unique for each concurrent *instance* of the same app on the same physical device (or emulator). NOTE: The uniqueness requirements are stronger (MUST) than the stability requirements (SHOULD). In particular, on platforms that allow multiple instances of the same app to run concurrently, requirements 1 and/or 2 MAY be violated in order to meet requirement 5. This will be relevant, for example, on desktop platforms. In an upcoming diff, we will pass device IDs meeting these criteria from both iOS and Android. Reviewed By: huntie, blakef Differential Revision: D49954920

Summary: Building on byCedric's approach in facebook/metro#991, and on D49954920, this diff passes stable, unique *logical device IDs* to the debugger connection infrastructure from Android and iOS. See D49954920 for the precise stability and uniqueness requirements that these IDs meet. Changelog: [Changed][General] - Automatically reconnect to an existing debugger session on relaunching the app Reviewed By: huntie Differential Revision: D49954919 fbshipit-source-id: 5796209efd0104f482d2a98c2616a163ea67ea61

Summary: Pull Request resolved: facebook#41080 Building on byCedric's approach in facebook/metro#991, adds support for passing a `device=...` argument to `/open-debugger` for more precise targeting. Changelog: [Internal] --- ## Note on what "device" means in this context In `dev-middleware` / `inspector-proxy`, "device" is something of a misnomer. It refers to a *logical device* containing one or more *pages*. In React Native, each app process forms its own logical device in which individual VMs register themselves as pages. An instance of `inspector-proxy` connects one or more *debuggers* (frontends) to one or more logical devices (one frontend to one page on one device). The intent of the logical device ID is to help with target discovery and especially *re*discovery - to reduce the number of times users need to explicitly close and restart the debugger frontend (e.g. after an app crash). If provided, the logical device ID: 1. SHOULD be stable for the current combination of physical device (or emulator instance) and app. 2. SHOULD be stable across installs/launches of the same app on the same device (or emulator instance), though it MAY be user-resettable (so as to not require any special privacy permissions). 3. MUST be unique across different apps on the same physical device (or emulator). 4. MUST be unique across physical devices (or emulators). 5. MUST be unique for each concurrent *instance* of the same app on the same physical device (or emulator). NOTE: The uniqueness requirements are stronger (MUST) than the stability requirements (SHOULD). In particular, on platforms that allow multiple instances of the same app to run concurrently, requirements 1 and/or 2 MAY be violated in order to meet requirement 5. This will be relevant, for example, on desktop platforms. In an upcoming diff, we will pass device IDs meeting these criteria from both iOS and Android. Reviewed By: huntie, blakef Differential Revision: D49954920 fbshipit-source-id: b37b90bea5b285a100b55cf44d6d21b4e7bd3e79

Summary: Building on byCedric's approach in facebook/metro#991, adds support for passing a `device=...` argument to `/open-debugger` for more precise targeting. Changelog: [Internal] --- ## Note on what "device" means in this context In `dev-middleware` / `inspector-proxy`, "device" is something of a misnomer. It refers to a *logical device* containing one or more *pages*. In React Native, each app process forms its own logical device in which individual VMs register themselves as pages. An instance of `inspector-proxy` connects one or more *debuggers* (frontends) to one or more logical devices (one frontend to one page on one device). The intent of the logical device ID is to help with target discovery and especially *re*discovery - to reduce the number of times users need to explicitly close and restart the debugger frontend (e.g. after an app crash). If provided, the logical device ID: 1. SHOULD be stable for the current combination of physical device (or emulator instance) and app. 2. SHOULD be stable across installs/launches of the same app on the same device (or emulator instance), though it MAY be user-resettable (so as to not require any special privacy permissions). 3. MUST be unique across different apps on the same physical device (or emulator). 4. MUST be unique across physical devices (or emulators). 5. MUST be unique for each concurrent *instance* of the same app on the same physical device (or emulator). NOTE: The uniqueness requirements are stronger (MUST) than the stability requirements (SHOULD). In particular, on platforms that allow multiple instances of the same app to run concurrently, requirements 1 and/or 2 MAY be violated in order to meet requirement 5. This will be relevant, for example, on desktop platforms. In an upcoming diff, we will pass device IDs meeting these criteria from both iOS and Android. Reviewed By: huntie, blakef Differential Revision: D49954920

…book#41152) Summary: Building on byCedric's approach in facebook/metro#991, and on D49954920, this diff passes stable, unique *logical device IDs* to the debugger connection infrastructure from Android and iOS. See D49954920 for the precise stability and uniqueness requirements that these IDs meet. Changelog: [Changed][General] - Automatically reconnect to an existing debugger session on relaunching the app Reviewed By: huntie Differential Revision: D49954919

Summary: Pull Request resolved: #41154 Pull Request resolved: #41080 Building on byCedric's approach in facebook/metro#991, adds support for passing a `device=...` argument to `/open-debugger` for more precise targeting. Changelog: [Internal] --- ## Note on what "device" means in this context In `dev-middleware` / `inspector-proxy`, "device" is something of a misnomer. It refers to a *logical device* containing one or more *pages*. In React Native, each app process forms its own logical device in which individual VMs register themselves as pages. An instance of `inspector-proxy` connects one or more *debuggers* (frontends) to one or more logical devices (one frontend to one page on one device). The intent of the logical device ID is to help with target discovery and especially *re*discovery - to reduce the number of times users need to explicitly close and restart the debugger frontend (e.g. after an app crash). If provided, the logical device ID: 1. SHOULD be stable for the current combination of physical device (or emulator instance) and app. 2. SHOULD be stable across installs/launches of the same app on the same device (or emulator instance), though it MAY be user-resettable (so as to not require any special privacy permissions). 3. MUST be unique across different apps on the same physical device (or emulator). 4. MUST be unique across physical devices (or emulators). 5. MUST be unique for each concurrent *instance* of the same app on the same physical device (or emulator). NOTE: The uniqueness requirements are stronger (MUST) than the stability requirements (SHOULD). In particular, on platforms that allow multiple instances of the same app to run concurrently, requirements 1 and/or 2 MAY be violated in order to meet requirement 5. This will be relevant, for example, on desktop platforms. In an upcoming diff, we will pass device IDs meeting these criteria from both iOS and Android. Reviewed By: huntie, blakef Differential Revision: D49954920 fbshipit-source-id: 45f2b50765dece34cbb93fa32abcdf3b0522391c

Summary: Pull Request resolved: #41152 Building on byCedric's approach in facebook/metro#991, and on D49954920, this diff passes stable, unique *logical device IDs* to the debugger connection infrastructure from Android and iOS. See D49954920 for the precise stability and uniqueness requirements that these IDs meet. Changelog: [Changed][General] - Automatically reconnect to an existing debugger session on relaunching the app Reviewed By: huntie Differential Revision: D49954919 fbshipit-source-id: d4d918f0cbfd9df426e888845817e00410efb9d3

Summary: Pull Request resolved: facebook#41154 Pull Request resolved: facebook#41080 Building on byCedric's approach in facebook/metro#991, adds support for passing a `device=...` argument to `/open-debugger` for more precise targeting. Changelog: [Internal] --- ## Note on what "device" means in this context In `dev-middleware` / `inspector-proxy`, "device" is something of a misnomer. It refers to a *logical device* containing one or more *pages*. In React Native, each app process forms its own logical device in which individual VMs register themselves as pages. An instance of `inspector-proxy` connects one or more *debuggers* (frontends) to one or more logical devices (one frontend to one page on one device). The intent of the logical device ID is to help with target discovery and especially *re*discovery - to reduce the number of times users need to explicitly close and restart the debugger frontend (e.g. after an app crash). If provided, the logical device ID: 1. SHOULD be stable for the current combination of physical device (or emulator instance) and app. 2. SHOULD be stable across installs/launches of the same app on the same device (or emulator instance), though it MAY be user-resettable (so as to not require any special privacy permissions). 3. MUST be unique across different apps on the same physical device (or emulator). 4. MUST be unique across physical devices (or emulators). 5. MUST be unique for each concurrent *instance* of the same app on the same physical device (or emulator). NOTE: The uniqueness requirements are stronger (MUST) than the stability requirements (SHOULD). In particular, on platforms that allow multiple instances of the same app to run concurrently, requirements 1 and/or 2 MAY be violated in order to meet requirement 5. This will be relevant, for example, on desktop platforms. In an upcoming diff, we will pass device IDs meeting these criteria from both iOS and Android. Reviewed By: huntie, blakef Differential Revision: D49954920 fbshipit-source-id: 45f2b50765dece34cbb93fa32abcdf3b0522391c

…book#41152) Summary: Pull Request resolved: facebook#41152 Building on byCedric's approach in facebook/metro#991, and on D49954920, this diff passes stable, unique *logical device IDs* to the debugger connection infrastructure from Android and iOS. See D49954920 for the precise stability and uniqueness requirements that these IDs meet. Changelog: [Changed][General] - Automatically reconnect to an existing debugger session on relaunching the app Reviewed By: huntie Differential Revision: D49954919 fbshipit-source-id: d4d918f0cbfd9df426e888845817e00410efb9d3

Summary: Pull Request resolved: facebook/react-native#41152 Building on byCedric's approach in facebook/metro#991, and on D49954920, this diff passes stable, unique *logical device IDs* to the debugger connection infrastructure from Android and iOS. See D49954920 for the precise stability and uniqueness requirements that these IDs meet. Changelog: [Changed][General] - Automatically reconnect to an existing debugger session on relaunching the app Reviewed By: huntie Differential Revision: D49954919 fbshipit-source-id: d4d918f0cbfd9df426e888845817e00410efb9d3

Summary: Pull Request resolved: #41152 Building on byCedric's approach in facebook/metro#991, and on D49954920, this diff passes stable, unique *logical device IDs* to the debugger connection infrastructure from Android and iOS. See D49954920 for the precise stability and uniqueness requirements that these IDs meet. Changelog: [Changed][General] - Automatically reconnect to an existing debugger session on relaunching the app Reviewed By: huntie Differential Revision: D49954919 fbshipit-source-id: d4d918f0cbfd9df426e888845817e00410efb9d3

Allow string device ids in inspector proxy

c52f355

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 27, 2023

Allow device identifiers when registering inspectable device

f2ca178

byCedric force-pushed the @bycedric/inspector-proxy/allow-device-identifiers branch from 1de8117 to f2ca178 Compare May 27, 2023 20:05

Fix some linting issue

8c4a827

huntie self-assigned this May 30, 2023

voideanvalue reviewed May 30, 2023

View reviewed changes

Move debugger socket reuse logic to device

9bbf992

byCedric force-pushed the @bycedric/inspector-proxy/allow-device-identifiers branch from adb4dec to 9bbf992 Compare June 2, 2023 11:14

huntie approved these changes Jun 2, 2023

View reviewed changes

packages/metro-inspector-proxy/src/Device.js Outdated Show resolved Hide resolved

packages/metro-inspector-proxy/src/InspectorProxy.js Outdated Show resolved Hide resolved

byCedric and others added 2 commits June 2, 2023 16:24

Update new device connection debug log

e4885b1

Co-authored-by: Alex Hunt <[email protected]>

Use docblock instead of line comments

1d516da

byCedric mentioned this pull request Jun 3, 2023

feature(cli): allow client-side device ids to reuse debugger sessions when restarting app expo/expo#22742

Merged

3 tasks

facebook-github-bot closed this in c6a94bc Jun 8, 2023

facebook-github-bot added the Merged label Jun 8, 2023

byCedric deleted the @bycedric/inspector-proxy/allow-device-identifiers branch August 14, 2023 13:53

blakef mentioned this pull request Oct 19, 2023

Enable launching debugger by device ID facebook/react-native#41080

Closed

motiz88 mentioned this pull request Oct 23, 2023

Use a stable device+app ID to register with the inspector proxy facebook/react-native#41152

Closed

motiz88 mentioned this pull request Oct 23, 2023

Enable launching debugger by device ID (#41080) facebook/react-native#41154

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow client-side device identifiers in inspector proxy #991

Allow client-side device identifiers in inspector proxy #991

byCedric commented May 27, 2023 •

edited

Loading

voideanvalue May 30, 2023

byCedric Jun 2, 2023

byCedric Jun 2, 2023 •

edited

Loading

huntie Jun 2, 2023 •

edited

Loading

huntie commented Jun 2, 2023 •

edited

Loading

byCedric commented Jun 2, 2023

motiz88 commented Jun 5, 2023

facebook-github-bot commented Jun 6, 2023

byCedric commented Jun 7, 2023

facebook-github-bot commented Jun 7, 2023

facebook-github-bot commented Jun 8, 2023

Allow client-side device identifiers in inspector proxy #991

Allow client-side device identifiers in inspector proxy #991

Conversation

byCedric commented May 27, 2023 • edited Loading

Summary

Next steps

Test plan

voideanvalue May 30, 2023

Choose a reason for hiding this comment

byCedric Jun 2, 2023

Choose a reason for hiding this comment

byCedric Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

huntie Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

huntie commented Jun 2, 2023 • edited Loading

byCedric commented Jun 2, 2023

motiz88 commented Jun 5, 2023

facebook-github-bot commented Jun 6, 2023

byCedric commented Jun 7, 2023

facebook-github-bot commented Jun 7, 2023

facebook-github-bot commented Jun 8, 2023

byCedric commented May 27, 2023 •

edited

Loading

byCedric Jun 2, 2023 •

edited

Loading

huntie Jun 2, 2023 •

edited

Loading

huntie commented Jun 2, 2023 •

edited

Loading