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

Jump to bottom

feature(cli): add custom inspector proxy based on metro-inspector-proxy #21449

Merged
merged 29 commits into from
Mar 8, 2023
Merged

feature(cli): add custom inspector proxy based on metro-inspector-proxy #21449

merged 29 commits into from
Mar 8, 2023

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Feb 28, 2023 •
edited
Loading

Why

Fixes ENG-7467

Related #21265

This is an initial draft to extend the CDP functionality of metro-inspector-proxy.

How

The implementation is slightly wonky around the ExpoInspectorDevice. We want to reuse as much as possible from metro-inspector-proxy, but we need to add stateful data per device.

In order to achieve that, we generate a new class type, based on the user's installed metro-inspector-proxy. This makes everything less readable but should include future updates in these classes.

As for the ExpoInspectorProxy, to avoid having to do the same thing, we just wrap the whole inspector class and reuse what we can. The device map is "linked" within the original inspector proxy instance, making the data available to all methods that need it.

Test Plan

Enable this feature with EXPO_USE_CUSTOM_INSPECTOR_PROXY=1

  • See tests for the actual CDP events we handle.
  • See tests on the "bootstrapping code" to create the inspector and devices.

Checklist

@linear
Copy link

linear bot commented Feb 28, 2023

ENG-7467 [network][dev-server] extend metro-inspector-proxy

To support more CDP commands, we should extend metro-inspector-proxy.

Also to support inspector network requests body, we should store requestId<->body map in dev-server.

https://chromedevtools.github.io/devtools-protocol/tot/Network/#method-getResponseBody

Screenshot 2023-02-02 at 10.48.44 AM.png

@byCedric byCedric marked this pull request as draft February 28, 2023 18:26
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Feb 28, 2023
@expo expo deleted a comment from expo-bot Feb 28, 2023
@expo expo deleted a comment from expo-bot Feb 28, 2023
Kudo
Kudo reviewed Mar 1, 2023
View reviewed changes
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

leaving some nit comments. most of code looks good to me. thanks Cedric!

packages/@expo/cli/src/start/server/metro/resolveFromProject.ts Show resolved Hide resolved
packages/@expo/cli/src/start/server/metro/inspector-proxy/index.ts Outdated Show resolved Hide resolved
@byCedric
Copy link
Member Author

byCedric commented Mar 2, 2023

Hold off on merging until #21475 is merged

@byCedric byCedric force-pushed the @bycedric/cli/experimental-inspector-proxy branch 2 times, most recently from 0b9f403 to cdf1f78 Compare March 2, 2023 14:09
@Kudo Kudo mentioned this pull request Mar 2, 2023
Merged
2 tasks
Kudo
Kudo reviewed Mar 2, 2023
View reviewed changes
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

leaving two problems from my testing. you could also test it on bare-expo with #21490.

be sure to set USE_DEV_CLIENT = true in MainApplication or useDevClient = true in AppDelegate.mm

packages/@expo/cli/src/start/server/metro/inspector-proxy/proxy.ts Outdated Show resolved Hide resolved
packages/@expo/cli/src/start/server/metro/inspector-proxy/device.ts Outdated Show resolved Hide resolved
byCedric and others added 12 commits March 6, 2023 13:37
@byCedric
chore(cli): add basic metro-inspector-proxy type definitions
0326125
@byCedric
feature(cli): add custom inspector proxy using metro-inspector-proxy
037afb3
@byCedric
feature(cli): add feature flag to enable experimental inspector proxy
5778a69
@byCedric
refactor(cli): integrate the custom inspector proxy when instantiatin…
24e6acc 
…g metro
@byCedric
test(cli): add network response handler tests
27e331e
@byCedric
refactor(cli): rename feature flag to EXPO_USE_CUSTOM_INSPECTOR_PROXY
9139808
@byCedric
chore(cli): add changelog entry
90b9d0a
@byCedric
test(cli): fix non-existing response assertion
b834f66
@byCedric
refactor(cli): add more debugging visibility and the continious messa…
5fe3b2c 
…ge stream
@byCedric
chore(cli): improve debugging visibility on socket close events
c41ba8f
@byCedric
refactor(cli): properly handle incorrect server addresses in inspector
6eb01c4
@byCedric
fix(cli): simplify inspector handlers and fix test feedback issues
900f695
@byCedric byCedric force-pushed the @bycedric/cli/experimental-inspector-proxy branch from faad89a to 63cd040 Compare March 6, 2023 12:56
@byCedric byCedric force-pushed the @bycedric/cli/experimental-inspector-proxy branch from 63cd040 to 142a2c8 Compare March 6, 2023 12:56
Kudo
Kudo approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

tested this pr a little bit and it works like a charm. i don't have any further comments and leave the remaining code style/testability for Evan to review.

@byCedric byCedric removed the bot: suggestions ExpoBot has some suggestions label Mar 8, 2023
@byCedric byCedric merged commit 5234fe3 into main Mar 8, 2023
@byCedric byCedric deleted the @bycedric/cli/experimental-inspector-proxy branch March 8, 2023 17:17
byCedric added a commit that referenced this pull request Mar 10, 2023
@byCedric @expo-bot
feature(cli): add vscode compatibility to inspector proxy (#21560)
b210db9 
# Why

Builds upon #21449

This adds a handler that makes sure the `vscode-js-debug` extension can
be used with the custom inspector proxy. These changes should NOT
interfere with the other debuggers.

# How

- Added vscode compatibility handler
- Add missing `description` to function nodes, as required by
vscode-js-debug
    _(before microsoft/vscode-js-debug#1583
- Respond to `Debugger.getPossibleBreakpoints`, Hermes doesn't respond
to this message
- ~~Filter out "useless" `callFrames` when debugging~~ 
  **Moved back to PR, we need to limit filtering**
  → #21663
- Avoid Hermes binding breakpoints to wrong locations, before sourcemap
is loaded
  → #21631
- Avoid Hermes crashing by disabling `Runtime.getProperties` on `symbol`
types
  → #21639

# Test Plan

- See tests
- ~~Test project with vscode, see [this
repository](https://github.com/byCedric/vscode-js-debug-issue-1583) for
a test project.~~
- See #21641 to test `apps/bare-expo`

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] 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 `expo prebuild` & EAS Build (eg:
updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
Kudo added a commit that referenced this pull request Apr 10, 2023
@Kudo
[dev-launcher] emits Expo(Network.receivedResponseBody) event (#21490)
9393d9a 
# Why

send to network response body for #21449

# How

send network response body `Expo(Network.receivedResponseBody)` event

# Test Plan

based on #21449 to check the network response

(cherry picked from commit 622cbd7)
Kudo added a commit that referenced this pull request Apr 10, 2023
@Kudo
[dev-launcher] emits Expo(Network.receivedResponseBody) event (#21490)
7d13e8e 
# Why

send to network response body for #21449

# How

send network response body `Expo(Network.receivedResponseBody)` event

# Test Plan

based on #21449 to check the network response

(cherry picked from commit 622cbd7)
Kudo added a commit that referenced this pull request Apr 10, 2023
@Kudo
[dev-launcher] emits Expo(Network.receivedResponseBody) event (#21490)
0560625 
# Why

send to network response body for #21449

# How

send network response body `Expo(Network.receivedResponseBody)` event

# Test Plan

based on #21449 to check the network response

(cherry picked from commit 622cbd7)
byCedric added a commit that referenced this pull request Apr 11, 2023
@byCedric @EvanBacon
feature(cli): add custom inspector proxy based on `metro-inspector-pr…
902c4b1 
…oxy` (#21449)

Fixes ENG-7467

Related #21265

This is an initial draft to extend the CDP functionality of
`metro-inspector-proxy`.

The implementation is slightly wonky around the `ExpoInspectorDevice`.
We want to reuse as much as possible from `metro-inspector-proxy`, but
we need to add stateful data per device.

In order to achieve that, we generate a new class type, based on the
user's installed `metro-inspector-proxy`. This makes everything less
readable but should include future updates in these classes.

As for the `ExpoInspectorProxy`, to avoid having to do the same thing,
we just wrap the whole inspector class and reuse what we can. The device
map is "linked" within the original inspector proxy instance, making the
data available to all methods that need it.

Enable this feature with `EXPO_USE_CUSTOM_INSPECTOR_PROXY=1`

- [x] See tests for the actual CDP events we handle.
- [ ] See tests on the "bootstrapping code" to create the inspector and
devices.

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] 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 `expo prebuild` & EAS Build (eg:
updated a module plugin).

---------

Co-authored-by: Evan Bacon <bacon@expo.io>
byCedric added a commit that referenced this pull request Apr 11, 2023
@byCedric @expo-bot
feature(cli): add vscode compatibility to inspector proxy (#21560)
9dc395a 
# Why

Builds upon #21449

This adds a handler that makes sure the `vscode-js-debug` extension can
be used with the custom inspector proxy. These changes should NOT
interfere with the other debuggers.

# How

- Added vscode compatibility handler
- Add missing `description` to function nodes, as required by
vscode-js-debug
    _(before microsoft/vscode-js-debug#1583
- Respond to `Debugger.getPossibleBreakpoints`, Hermes doesn't respond
to this message
- ~~Filter out "useless" `callFrames` when debugging~~ 
  **Moved back to PR, we need to limit filtering**
  → #21663
- Avoid Hermes binding breakpoints to wrong locations, before sourcemap
is loaded
  → #21631
- Avoid Hermes crashing by disabling `Runtime.getProperties` on `symbol`
types
  → #21639

# Test Plan

- See tests
- ~~Test project with vscode, see [this
repository](https://github.com/byCedric/vscode-js-debug-issue-1583) for
a test project.~~
- See #21641 to test `apps/bare-expo`

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] 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 `expo prebuild` & EAS Build (eg:
updated a module plugin).

---------

Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
@Kudo Kudo added the published Changes from the PR have been published to npm label Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kudo Kudo Kudo approved these changes

@EvanBacon EvanBacon EvanBacon approved these changes

@Simek Simek Awaiting requested review from Simek Simek is a code owner

@amandeepmittal amandeepmittal Awaiting requested review from amandeepmittal amandeepmittal is a code owner

@kbrandwijk kbrandwijk Awaiting requested review from kbrandwijk

Assignees
No one assigned
Labels
published Changes from the PR have been published to npm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@byCedric @Kudo @EvanBacon @expo-bot