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.

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

Fix Web Debugging and Other Websocket Endpoints with Metro 0.67 #1560

Merged

Conversation

NickGerleman
Copy link
Contributor

Summary:

Metro 0.67 included facebook/metro@38a200e which changed the way external applications should interface with it to add new WebSocket endpoints. This materializes as failures when web debugging RN 0.68 apps.

This change applies the recommended fix, of passing a list of non-server endpoints to Metro when starting the server. ws is updated as part of the change, along with removing type-checker suppressions, of which one was resulting in a runtime error.

Test Plan:

Tested web debugging after replacing the relevant CLI packages in node_modules, with the edited ones.

Metro 0.67 included facebook/metro@38a200e which changed the way external applications should interface with it to add new WebSocket endpoints. This materializes as failures when web debugging RN 0.68 apps.

This change applies the reccomended fix, of passing a list of non-server endpoints to Metro when starting the server. I verified that web debugging works correctly after making the fix, for the same app where it previously did not work.

`ws` is updated as part of the change, along with removing tpe-checker suppressions, of which one was resulting in a runtime error.
@NickGerleman
Copy link
Contributor Author

This fixes web debugging, but the PR in the current state has added constant warnings logged to console, that I want to take a look at before this is merged.
image

@NickGerleman NickGerleman changed the title Fix Web Debugging and Other Websocket Endpoints with Metro 0.67 [WIP] Fix Web Debugging and Other Websocket Endpoints with Metro 0.67 Feb 18, 2022
@NickGerleman
Copy link
Contributor Author

The next time I launched it, I didn't see the warnings, but the debugger count in the UI was way off.
image

@NickGerleman
Copy link
Contributor Author

NickGerleman commented Feb 18, 2022

That number seems to not actually be controlled by us, looking at packages\debugger-ui\src\ui\index.js. Going to just remove it from the UI, if it isn't always stable, and everything else seems to work.

@NickGerleman
Copy link
Contributor Author

NickGerleman commented Feb 18, 2022

This fixes web debugging, but the PR in the current state has added constant warnings logged to console, that I want to take a look at before this is merged.

I tried running Metro, then web debugging, a few times, even after resetting the cache, and haven't seen the issue since. Not sure if I had some stale sate, or if there's a real issue here...

@NickGerleman NickGerleman changed the title [WIP] Fix Web Debugging and Other Websocket Endpoints with Metro 0.67 Fix Web Debugging and Other Websocket Endpoints with Metro 0.67 Feb 18, 2022
@NickGerleman NickGerleman marked this pull request as ready for review February 18, 2022 01:40
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks, this is great <3! I've moved the creation of the sockets and debugger proxy up and removed the stubs for isDebuggerConnected and broadcast, since now they're eagerly and always (I think?) created.

Tested various cases, disconnecting the debugger and app and server many times, and it seem to work as expected every time. Going to merge this in and release asap

@thymikee thymikee merged commit e4b7805 into react-native-community:master Feb 18, 2022
thymikee added a commit that referenced this pull request Feb 18, 2022
* Fix Web Debugging and Other Websocket Endpoints with Metro 0.67

Metro 0.67 included facebook/metro@38a200e which changed the way external applications should interface with it to add new WebSocket endpoints. This materializes as failures when web debugging RN 0.68 apps.

This change applies the reccomended fix, of passing a list of non-server endpoints to Metro when starting the server. I verified that web debugging works correctly after making the fix, for the same app where it previously did not work.

`ws` is updated as part of the change, along with removing tpe-checker suppressions, of which one was resulting in a runtime error.

* Remove session number

* chore: stop using stubs for isDebuggerConnected and broadcast

Co-authored-by: Michał Pierzchała <thymikee@gmail.com>
@thymikee thymikee mentioned this pull request Feb 18, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 23, 2022
…#33156)

Summary:
Doing this patch level bump to ensure that all packages consuming `react-native` will get `7.0.3` and not lower 7.x versions.
This is because this new patch contains this fix:  react-native-community/cli#1560
(thanks NickGerleman & thymikee for your work!)

We'll have to cherry-pick this into the 0.68 branch.

While at it, I've also done a cheeky `npx yarn-deduplicate` to clean up the `yarn.lock` a bit.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Changed] - Bump RN CLI to v7.0.3 to address web debugging issue

Pull Request resolved: #33156

Test Plan:
CI + some local testing via `test-manual-e2e`:
<img width="1779" alt="Screenshot 2022-02-21 at 11 40 54" src="https://user-images.githubusercontent.com/16104054/154948695-8c40bb56-87eb-4326-a740-49930994c08b.png">

Reviewed By: cortinico

Differential Revision: D34385503

Pulled By: motiz88

fbshipit-source-id: f0d8c4e0e92f83c0d819eeaa0fbec27820145968
ShikaSD pushed a commit to facebook/react-native that referenced this pull request Feb 24, 2022
…#33156)

Summary:
Doing this patch level bump to ensure that all packages consuming `react-native` will get `7.0.3` and not lower 7.x versions.
This is because this new patch contains this fix:  react-native-community/cli#1560
(thanks NickGerleman & thymikee for your work!)

We'll have to cherry-pick this into the 0.68 branch.

While at it, I've also done a cheeky `npx yarn-deduplicate` to clean up the `yarn.lock` a bit.

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Changed] - Bump RN CLI to v7.0.3 to address web debugging issue

Pull Request resolved: #33156

Test Plan:
CI + some local testing via `test-manual-e2e`:
<img width="1779" alt="Screenshot 2022-02-21 at 11 40 54" src="https://user-images.githubusercontent.com/16104054/154948695-8c40bb56-87eb-4326-a740-49930994c08b.png">

Reviewed By: cortinico

Differential Revision: D34385503

Pulled By: motiz88

fbshipit-source-id: f0d8c4e0e92f83c0d819eeaa0fbec27820145968
ShikaSD pushed a commit to facebook/react-native that referenced this pull request Feb 24, 2022
…#33156)

Summary:
Doing this patch level bump to ensure that all packages consuming `react-native` will get `7.0.3` and not lower 7.x versions.
This is because this new patch contains this fix:  react-native-community/cli#1560
(thanks NickGerleman & thymikee for your work!)

We'll have to cherry-pick this into the 0.68 branch.

While at it, I've also done a cheeky `npx yarn-deduplicate` to clean up the `yarn.lock` a bit.

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Changed] - Bump RN CLI to v7.0.3 to address web debugging issue

Pull Request resolved: #33156

Test Plan:
CI + some local testing via `test-manual-e2e`:
<img width="1779" alt="Screenshot 2022-02-21 at 11 40 54" src="https://user-images.githubusercontent.com/16104054/154948695-8c40bb56-87eb-4326-a740-49930994c08b.png">

Reviewed By: cortinico

Differential Revision: D34385503

Pulled By: motiz88

fbshipit-source-id: f0d8c4e0e92f83c0d819eeaa0fbec27820145968
@grabbou
Copy link
Member

grabbou commented Feb 25, 2022

Sorry for being late here, but why e4b7805#diff-419c2d522450d9ec89f017caa5152add5b0fa81d97b476fe461404aafad5e1dfL52 was removed? Any particular reason?

@NickGerleman
Copy link
Contributor Author

The next time I launched it, I didn't see the warnings, but the debugger count in the UI was way off. image

That number seems to not actually be controlled by us, looking at packages\debugger-ui\src\ui\index.js. Going to just remove it from the UI, if it isn't always stable, and everything else seems to work.

The number seemed to correspond to an opaque ID given to the debugger UI by Metro. After the Metro update and networking changes, the number would fluctuate.

Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…facebook#33156)

Summary:
Doing this patch level bump to ensure that all packages consuming `react-native` will get `7.0.3` and not lower 7.x versions.
This is because this new patch contains this fix:  react-native-community/cli#1560
(thanks NickGerleman & thymikee for your work!)

We'll have to cherry-pick this into the 0.68 branch.

While at it, I've also done a cheeky `npx yarn-deduplicate` to clean up the `yarn.lock` a bit.

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Changed] - Bump RN CLI to v7.0.3 to address web debugging issue

Pull Request resolved: facebook#33156

Test Plan:
CI + some local testing via `test-manual-e2e`:
<img width="1779" alt="Screenshot 2022-02-21 at 11 40 54" src="https://user-images.githubusercontent.com/16104054/154948695-8c40bb56-87eb-4326-a740-49930994c08b.png">

Reviewed By: cortinico

Differential Revision: D34385503

Pulled By: motiz88

fbshipit-source-id: f0d8c4e0e92f83c0d819eeaa0fbec27820145968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants