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

DevTools: fix backend activation #26779

Merged
merged 2 commits into from
May 4, 2023
Merged

DevTools: fix backend activation #26779

merged 2 commits into from
May 4, 2023

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented May 4, 2023 •
edited
Loading

Summary

We have a case:

  1. Open components tab
  2. Close Chrome / Firefox devtools window completely
  3. Reopen browser devtools panel
  4. Open components tab

Currently, in version 4.27.6, we cannot load the components tree.

This PR contains two changes:

How did you test this change?

This fixes the case mentioned prior. Currently in 4.27.6 version it is not working, we need to refresh the page to make it work.

I've tested this in several environments: chrome, firefox, standalone with RN application.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 4, 2023
hoxyq
hoxyq commented May 4, 2023
View reviewed changes
packages/react-devtools-shared/src/backend/index.js
Comment on lines -67 to -70
// skip if already attached
if (renderer.attached) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Frontend side of devtools needs renderer-attached event to be emitted. With this check this will never happen and agent will not receive operations from hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @mondaychen

Are there any cases I am missing where we really need this check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I think this was something I missed. Let's remove it now.

@hoxyq hoxyq marked this pull request as ready for review May 4, 2023 16:34
@hoxyq hoxyq merged commit 377c517 into facebook:main May 4, 2023
@hoxyq hoxyq mentioned this pull request May 4, 2023
Merged
@hoxyq hoxyq deleted the devtools/fix-backend-activation branch May 4, 2023 17:23
hoxyq added a commit that referenced this pull request May 4, 2023
@hoxyq
React DevTools 4.27.6 -> 4.27.7 (#26780)
783e7fc 
List of changes:
* DevTools: fix backend activation ([hoxyq](https://github.com/hoxyq) in
[#26779](#26779))
* fix[dynamic-scripts-injection]: unregister content scripts before
registration ([hoxyq](https://github.com/hoxyq) in
[#26765](#26765))
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request May 5, 2023
@hoxyq @facebook-github-bot
React DevTools 4.27.6 -> 4.27.7 (#26780)
33812e9 
Summary:
List of changes:
* DevTools: fix backend activation ([hoxyq](https://github.com/hoxyq) in
[#26779](facebook/react#26779))
* fix[dynamic-scripts-injection]: unregister content scripts before
registration ([hoxyq](https://github.com/hoxyq) in
[#26765](facebook/react#26765))

DiffTrain build for commit facebook/react@783e7fc.

Changelog: [Internal]

<< DO NOT EDIT BELOW THIS LINE >>

Reviewed By: sammy-SC

Differential Revision: D45575104

Pulled By: tyao1

fbshipit-source-id: c065eb8f1f200170f611b5becc0332109f22ef71
acdlite added a commit to acdlite/next.js that referenced this pull request May 7, 2023
@acdlite
Upgrade React to 18.3.0-canary-16d053d59-20230506
424cf23 
Fixes a bug where `useFormStatus` crashed during SSR.

Includes the following upstream changes:

- [16d053d59](https://github.com/facebook/react/commits/16d053d59) Add useFormStatus to server rendering stub ([vercel#26788](facebook/react#26788)) (Andrew Clark)
- [efb381bbf](https://github.com/facebook/react/commits/efb381bbf) [Release Script] Print a hint where to get the token ([vercel#26783](facebook/react#26783)) (dan)
- [b00e27342](https://github.com/facebook/react/commits/b00e27342) Use native scheduler if defined in global scope ([vercel#26554](facebook/react#26554)) (Samuel Susla)
- [783e7fcfa](https://github.com/facebook/react/commits/783e7fcfa) React DevTools 4.27.6 -> 4.27.7 ([vercel#26780](facebook/react#26780)) (Ruslan Lesiutin)
- [377c5175f](https://github.com/facebook/react/commits/377c5175f) DevTools: fix backend activation ([vercel#26779](facebook/react#26779)) (Ruslan Lesiutin)
@acdlite acdlite mentioned this pull request May 7, 2023
Merged
jeongshin pushed a commit to jeongshin/react-native that referenced this pull request May 7, 2023
@hoxyq @jeongshin
React DevTools 4.27.6 -> 4.27.7 (facebook#26780)
375f933 
Summary:
List of changes:
* DevTools: fix backend activation ([hoxyq](https://github.com/hoxyq) in
[facebook#26779](facebook/react#26779))
* fix[dynamic-scripts-injection]: unregister content scripts before
registration ([hoxyq](https://github.com/hoxyq) in
[facebook#26765](facebook/react#26765))

DiffTrain build for commit facebook/react@783e7fc.

Changelog: [Internal]

<< DO NOT EDIT BELOW THIS LINE >>

Reviewed By: sammy-SC

Differential Revision: D45575104

Pulled By: tyao1

fbshipit-source-id: c065eb8f1f200170f611b5becc0332109f22ef71
acdlite added a commit to acdlite/next.js that referenced this pull request May 7, 2023
@acdlite
Upgrade React to 18.3.0-canary-16d053d59-20230506
123efcf 
Fixes a bug where `useFormStatus` crashed during SSR.

Includes the following upstream changes:

- [16d053d59](https://github.com/facebook/react/commits/16d053d59) Add useFormStatus to server rendering stub ([vercel#26788](facebook/react#26788)) (Andrew Clark)
- [efb381bbf](https://github.com/facebook/react/commits/efb381bbf) [Release Script] Print a hint where to get the token ([vercel#26783](facebook/react#26783)) (dan)
- [b00e27342](https://github.com/facebook/react/commits/b00e27342) Use native scheduler if defined in global scope ([vercel#26554](facebook/react#26554)) (Samuel Susla)
- [783e7fcfa](https://github.com/facebook/react/commits/783e7fcfa) React DevTools 4.27.6 -> 4.27.7 ([vercel#26780](facebook/react#26780)) (Ruslan Lesiutin)
- [377c5175f](https://github.com/facebook/react/commits/377c5175f) DevTools: fix backend activation ([vercel#26779](facebook/react#26779)) (Ruslan Lesiutin)
acdlite added a commit to acdlite/next.js that referenced this pull request May 7, 2023
@acdlite
Upgrade React to 18.3.0-canary-16d053d59-20230506
8b0485e 
Fixes a bug where `useFormStatus` crashed during SSR.

Includes the following upstream changes:

- [16d053d59](https://github.com/facebook/react/commits/16d053d59) Add useFormStatus to server rendering stub ([vercel#26788](facebook/react#26788)) (Andrew Clark)
- [efb381bbf](https://github.com/facebook/react/commits/efb381bbf) [Release Script] Print a hint where to get the token ([vercel#26783](facebook/react#26783)) (dan)
- [b00e27342](https://github.com/facebook/react/commits/b00e27342) Use native scheduler if defined in global scope ([vercel#26554](facebook/react#26554)) (Samuel Susla)
- [783e7fcfa](https://github.com/facebook/react/commits/783e7fcfa) React DevTools 4.27.6 -> 4.27.7 ([vercel#26780](facebook/react#26780)) (Ruslan Lesiutin)
- [377c5175f](https://github.com/facebook/react/commits/377c5175f) DevTools: fix backend activation ([vercel#26779](facebook/react#26779)) (Ruslan Lesiutin)
kodiakhq bot pushed a commit to vercel/next.js that referenced this pull request May 7, 2023
@acdlite
Upgrade React to 18.3.0-canary-16d053d59-20230506 (#49402)
7e02f11 
Fixes a bug where `useFormStatus` crashed during SSR.

Includes the following upstream changes:

- [16d053d59](https://github.com/facebook/react/commits/16d053d59) Add useFormStatus to server rendering stub ([#26788](facebook/react#26788)) (Andrew Clark)
- [efb381bbf](https://github.com/facebook/react/commits/efb381bbf) [Release Script] Print a hint where to get the token ([#26783](facebook/react#26783)) (dan)
- [b00e27342](https://github.com/facebook/react/commits/b00e27342) Use native scheduler if defined in global scope ([#26554](facebook/react#26554)) (Samuel Susla)
- [783e7fcfa](https://github.com/facebook/react/commits/783e7fcfa) React DevTools 4.27.6 -> 4.27.7 ([#26780](facebook/react#26780)) (Ruslan Lesiutin)
- [377c5175f](https://github.com/facebook/react/commits/377c5175f) DevTools: fix backend activation ([#26779](facebook/react#26779)) (Ruslan Lesiutin)
@hoxyq hoxyq mentioned this pull request May 12, 2023
Merged
hoxyq added a commit that referenced this pull request May 12, 2023
@hoxyq
fix[devtools]: fixed duplicated backend activation with multiple rend…
67a05d0 
…erers (#26807)

## Summary
Initially reported in #26797.
Was not able to reproduce the exact same problem, but found this case:

1. Open corresponding codepen from the issue in debug mode
2. Open components tab of the extension
3. Refresh the page

Received multiple errors:
- Warning in the Console tab: Invalid renderer id "2".
- Error in the Components tab: Uncaught Error: Cannot add node "3"
because a node with that id is already in the Store.

This problem has occurred after landing a fix in
#26779. Looks like Chrome is
keeping the injected scripts (the backend in this case) and we start
backend twice.
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request May 16, 2023
@hoxyq @facebook-github-bot
fix[devtools]: fixed duplicated backend activation with multiple rend…
ada6c51 
…erers (#26807)

Summary:
## Summary
Initially reported in facebook/react#26797.
Was not able to reproduce the exact same problem, but found this case:

1. Open corresponding codepen from the issue in debug mode
2. Open components tab of the extension
3. Refresh the page

Received multiple errors:
- Warning in the Console tab: Invalid renderer id "2".
- Error in the Components tab: Uncaught Error: Cannot add node "3"
because a node with that id is already in the Store.

This problem has occurred after landing a fix in
facebook/react#26779. Looks like Chrome is
keeping the injected scripts (the backend in this case) and we start
backend twice.

DiffTrain build for commit facebook/react@67a05d0.

Reviewed By: javache

Differential Revision: D45815160

Pulled By: hoxyq

fbshipit-source-id: 99f3c45009b07a0fb89f0674ccfb24f170cdb29d
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@hoxyq @AndyPengc12
DevTools: fix backend activation (facebook#26779)
6a1bd52 
## Summary
We have a case:
1. Open components tab
2. Close Chrome / Firefox devtools window completely
3. Reopen browser devtools panel
4. Open components tab

Currently, in version 4.27.6, we cannot load the components tree.

This PR contains two changes:
- non-functional refactoring in
`react-devtools-shared/src/devtools/store.js`: removed some redundant
type castings.
- fixed backend manager logic (introduced in
facebook#26615) to activate already
registered backends. Looks like frontend of devtools also depends on
`renderer-attached` event, without it component tree won't load.

## How did you test this change?
This fixes the case mentioned prior. Currently in 4.27.6 version it is
not working, we need to refresh the page to make it work.

I've tested this in several environments: chrome, firefox, standalone
with RN application.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@hoxyq @AndyPengc12
React DevTools 4.27.6 -> 4.27.7 (facebook#26780)
9db2238 
List of changes:
* DevTools: fix backend activation ([hoxyq](https://github.com/hoxyq) in
[facebook#26779](facebook#26779))
* fix[dynamic-scripts-injection]: unregister content scripts before
registration ([hoxyq](https://github.com/hoxyq) in
[facebook#26765](facebook#26765))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
@hoxyq @AndyPengc12
fix[devtools]: fixed duplicated backend activation with multiple rend…
efbe7de 
…erers (facebook#26807)

## Summary
Initially reported in facebook#26797.
Was not able to reproduce the exact same problem, but found this case:

1. Open corresponding codepen from the issue in debug mode
2. Open components tab of the extension
3. Refresh the page

Received multiple errors:
- Warning in the Console tab: Invalid renderer id "2".
- Error in the Components tab: Uncaught Error: Cannot add node "3"
because a node with that id is already in the Store.

This problem has occurred after landing a fix in
facebook#26779. Looks like Chrome is
keeping the injected scripts (the backend in this case) and we start
backend twice.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
@hoxyq
DevTools: fix backend activation (#26779)
7216e1f 
## Summary
We have a case:
1. Open components tab
2. Close Chrome / Firefox devtools window completely
3. Reopen browser devtools panel
4. Open components tab

Currently, in version 4.27.6, we cannot load the components tree.

This PR contains two changes:
- non-functional refactoring in
`react-devtools-shared/src/devtools/store.js`: removed some redundant
type castings.
- fixed backend manager logic (introduced in
#26615) to activate already
registered backends. Looks like frontend of devtools also depends on
`renderer-attached` event, without it component tree won't load.

## How did you test this change?
This fixes the case mentioned prior. Currently in 4.27.6 version it is
not working, we need to refresh the page to make it work.

I've tested this in several environments: chrome, firefox, standalone
with RN application.

DiffTrain build for commit 377c517.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mondaychen mondaychen mondaychen left review comments

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoxyq @mondaychen @facebook-github-bot