feature(cli): add vscode compatibility handler to inspector proxy #21560
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
byCedric
force-pushed
the
@bycedric/cli/experimental-inspector-proxy-vscode-compat
branch
from
530b4c7
to
ef9b38b
Compare
EvanBacon
reviewed
Mar 6, 2023
...ges/@expo/cli/src/start/server/metro/inspector-proxy/messages/__tests__/VscodeCompat.test.ts
Outdated
Show resolved
Hide resolved
EvanBacon
reviewed
Mar 6, 2023
packages/@expo/cli/src/start/server/metro/inspector-proxy/messages/VscodeCompat.ts
Outdated
Show resolved
Hide resolved
EvanBacon
approved these changes
Mar 6, 2023
byCedric
force-pushed
the
@bycedric/cli/experimental-inspector-proxy-vscode-compat
branch
from
a11aa42
to
4edd57b
Compare
Base automatically changed from
@bycedric/cli/experimental-inspector-proxy
to
main
March 8, 2023 17:16
byCedric
requested review from
Simek,
amandeepmittal and
kbrandwijk
as code owners
byCedric
force-pushed
the
@bycedric/cli/experimental-inspector-proxy-vscode-compat
branch
from
4edd57b
to
52711b3
Compare
expo-bot
reviewed
Mar 8, 2023
byCedric
force-pushed
the
@bycedric/cli/experimental-inspector-proxy-vscode-compat
branch
from
eea9ee2
to
44d6104
Compare
expo-bot
reviewed
Mar 8, 2023
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
byCedric
force-pushed
the
@bycedric/cli/experimental-inspector-proxy-vscode-compat
branch
from
9e310c6
to
4c8918d
Compare
…ages (#21566) # Why We don't need to show `callFrames` which are not traceable. before | after --- | --- <img alt="image" src="https://user-images.githubusercontent.com/1203991/223199631-fa5b5f25-733b-468d-b158-8e70b88c2b3d.png"> | <img alt="image" src="https://user-images.githubusercontent.com/1203991/223199486-22496281-3de7-4693-a47b-63ec168762c8.png"> > ℹ️ **FunFact** > The Chrome DevTools already filters these out from the `callFrames` # How - Filter `callFrames` in the `Debugger.paused` event # Test Plan - See added test # 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).
This was referenced Mar 9, 2023
…setBreakpointByUrl` (#21631) # Why Vscode is eagerly trying to create breakpoints, even before the sourcemap is properly loaded. It uses the [`Debugger.setBreakpointyUrl`](https://chromedevtools.github.io/devtools-protocol/v8/Debugger/#method-setBreakpointByUrl), with a `urlRegex`, which confuses Hermes. > Vscode also incorrectly adds `file://` to the regex, already containing a URL with protocol (e.g. `file://http://localhost/App.js`). This is used to detect these specific breakpoint requests coming from vscode. My understanding is that Hermes is notifying the debugger of the loaded bundle (including the sourcemap reference). After this notification, the debuggers load the full sourcemap to understand the code that's being executed. It looks like Hermes is using the `lineNumber` and `columnNumber` from the actual bundle, resulting in `App.js:15` being translated to `index.bundle:15`. # How This intercepts these invalid `urlRegex` from vscode, and asks Hermes to create an unbounded breakpoint by avoiding `urlRegex`. After the sourcemap is loaded in vscode, the unbounded breakpoint is reattempted through `Debugger.setBreakpoint` using a `scriptId` that vscode resolves using the proper `localRoot`/`remoteRoot` settings. before | after --- | ---  |  # Test Plan - Clone https://github.com/byCedric/vscode-js-debug-issue-1583 - Set a breakpoint in `App.js` before connecting to Hermes - The location of the breakpoint should not change # 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).
…1639) # Why Remove `objectId` from symbol types to disable vscode to improperly fetch object properties on symbols, causing Hermes to hard-crash. vscode before | vsocde after --- | ---  |  chrome before | chrome after --- | ---  |  # How By removing the `objectId`, the vscode debugger doesn't have any reference to request the `Runtime.getProperties` on the symbol itself. This avoids being able to crash Hermes by running `Runtime.getProperties` on `symbol` types. # Test Plan - Clone https://github.com/byCedric/vscode-js-debug-issue-1583 - Add `const testSymbol = Symbol('test');` near a breakpoint - Try to expand the `testSymbol` in the variables explorer # 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).
3 tasks
Kudo
reviewed
Mar 9, 2023
packages/@expo/cli/src/start/server/metro/inspector-proxy/handlers/VscodeCompat.ts
Outdated
Show resolved
Hide resolved
|
@Kudo @EvanBacon I'm merging this to main, with the exception of the |
byCedric
deleted the
@bycedric/cli/experimental-inspector-proxy-vscode-compat
branch
byCedric
added a commit
that referenced
this pull request
Apr 11, 2023
# 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Builds upon #21449
This adds a handler that makes sure the
vscode-js-debugextension can be used with the custom inspector proxy. These changes should NOT interfere with the other debuggers.How
descriptionto function nodes, as required by vscode-js-debug(before Object previews are not devtools spec compliant microsoft/vscode-js-debug#1583)
Debugger.getPossibleBreakpoints, Hermes doesn't respond to this messageFilter out "useless"callFrameswhen debuggingMoved back to PR, we need to limit filtering
→ fix(cli): filter "useless" native
callFramesfrom callstack #21663→ fix(cli): avoid hermes binding breakpoints to wrong locations using
setBreakpointByUrl#21631Runtime.getPropertiesonsymboltypes→ fix(cli): remove symbol objectIds to avoid vscode crashing hermes #21639
Test Plan
Test project with vscode, see this repository for a test project.bare-expofor vscode debugging #21641 to testapps/bare-expoChecklist
expo prebuild& EAS Build (eg: updated a module plugin).