fix(dev-middleware): allow inspector proxy to fetch sourcemaps on lan connections #43307
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
facebook-github-bot
added
CLA Signed
byCedric
force-pushed
the
@bycedric/dev-middleware/inspector-sourcemaps-lan
branch
from
3f22782
to
ee560aa
Compare
|
@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
4 tasks
robhogan
requested changes
Mar 8, 2024
There was a problem hiding this comment.
Hi @byCedric, sorry for the delay with this, I see you'd like to get it landed before Monday's RC.
The change looks good but the new test is currently failing internal CI because those machines don't have local IPv4 - and more generally, it could fail on routing or firewall rules in exotic setups.
I'd be OK just removing the test tbh - it's made its point by passing on CircleCI, and the new debugger (with Network.loadNetworkResource) should replace the need for any of this anyway.
|
@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
robhogan
approved these changes
Mar 11, 2024
huntie
pushed a commit
that referenced
this pull request
Mar 18, 2024
… connections (#43307) Summary: The inspector proxy is inlining source maps on `Debugger.scriptParsed` CDP events. The inlining prevents Chrome DevTools from downloading this remotely, as that's not supported in newer versions anymore. The current implementation locks this inlining mechanism to just `localhost` and/or `127.0.0.1` addresses, making it incompatible with LAN or tunnel device connections. This PR removes that limitation to allow source map inlining on these LAN and tunnel connections. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [GENERAL][FIXED] Allow Inspector proxy to inline source maps on LAN connections Pull Request resolved: #43307 Test Plan: - See added test - Start Metro and connect a device over LAN, open the chrome devtools Reviewed By: huntie Differential Revision: D54485247 Pulled By: robhogan fbshipit-source-id: 6fcb0c6dd762d2f0a013497ba0a1126095b9130b
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 20, 2024
…#43559) Summary: This is a follow-up bugfix for expo/expo#27425, related to: - #43291 - #43307 - #43310 - #43364 The middleware API works as intended and can run our extended CDP events. Unfortunately, this only applies to an actual `Page` from the device, not for the `React Native Experimental (Improved Chrome Reloads)` synthetic / virtual page. That's because the middleware instantiation gets aborted when the page can't be found in `this.#pages.get(pageId)`, which always returns `null` for this synthetic page. ## Changelog: [GENERAL] [FIXED] Create custom message handler for synthetic page Pull Request resolved: #43559 Test Plan: See added test case. Reviewed By: motiz88 Differential Revision: D55129412 Pulled By: huntie fbshipit-source-id: 9679d8fe68f3cb4104f4a042f93612b995baddc9
huntie
pushed a commit
that referenced
this pull request
Mar 21, 2024
…#43559) Summary: This is a follow-up bugfix for expo/expo#27425, related to: - #43291 - #43307 - #43310 - #43364 The middleware API works as intended and can run our extended CDP events. Unfortunately, this only applies to an actual `Page` from the device, not for the `React Native Experimental (Improved Chrome Reloads)` synthetic / virtual page. That's because the middleware instantiation gets aborted when the page can't be found in `this.#pages.get(pageId)`, which always returns `null` for this synthetic page. ## Changelog: [GENERAL] [FIXED] Create custom message handler for synthetic page Pull Request resolved: #43559 Test Plan: See added test case. Reviewed By: motiz88 Differential Revision: D55129412 Pulled By: huntie fbshipit-source-id: 9679d8fe68f3cb4104f4a042f93612b995baddc9
This was referenced Apr 5, 2024
This was referenced Jun 28, 2024
Merged
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.
Summary:
The inspector proxy is inlining source maps on
Debugger.scriptParsedCDP events. The inlining prevents Chrome DevTools from downloading this remotely, as that's not supported in newer versions anymore.The current implementation locks this inlining mechanism to just
localhostand/or127.0.0.1addresses, making it incompatible with LAN or tunnel device connections.This PR removes that limitation to allow source map inlining on these LAN and tunnel connections.
Changelog:
[GENERAL][FIXED] Allow Inspector proxy to inline source maps on LAN connections
Test Plan: