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

refactor[devtools/extension]: migrate from using setInterval for polling if react is loaded #27323

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Sep 1, 2023

chrome.devtools.inspectedWindow.eval is asynchronous, so using it in setInterval is a mistake.
Sometimes this results into mounting React DevTools twice, and user sees errors about duplicated fibers in store.

With these changes, executeIfReactHasLoaded executed recursively with a threshold (in case if page doesn't have react).

Although we minimize the risk of mounting DevTools twice here, this approach is not the best way to have this problem solved. Dumping some thoughts and ideas that I've tried, but which are out of the scope for this release, because they can be too risky and time-consuming.
Potential changes:

  • Have 2 content scripts:
    • One prepareInjection to notify service worker on renderer attached
    • One which runs on document_idle to finalize check, in case if there is no react
  • Service worker will notify devtools page that it is ready to mount React DevTools panels or should show that there is no React to be found
  • Extension port from devtools page should be persistent and connected when main.js is executed
  • Might require refactoring the logic of how we connect devtools and proxy ports

Some corner cases:

  • Navigating to restricted pages, like chrome://<something> and back
  • When react is lazily loaded, like in an attached iframe, or just opened modal
  • In-tab navigation with pre-cached pages, I think only Chrome does it
  • Firefox is still on manifest v2 and it doesn't allow running content scripts in ExecutionWorld.MAIN, so it requires a different approach

@hoxyq hoxyq requested a review from gsathya September 1, 2023 14:42
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 1, 2023
@hoxyq hoxyq merged commit 9b4f847 into facebook:main Sep 1, 2023
2 checks passed
@hoxyq hoxyq deleted the devtools/refactor-react-detection-logic branch September 1, 2023 15:23
hoxyq added a commit that referenced this pull request Sep 5, 2023
This is a patch version to fix some bugs in a previous internal release.
I am expecting this one also to be internal-only, need to make sure that
extension is stable in Chrome. Some changes and improvements are
expected for Firefox, though, before going public.

* refactor[devtools/extension]: handle ports disconnection, instead of
frequent reconnection ([hoxyq](https://github.com/hoxyq) in
[#27336](#27336))
* refactor[devtools/extension]: migrate from using setInterval for
polling if react is loaded ([hoxyq](https://github.com/hoxyq) in
[#27323](#27323))
* fix[devtools/extension]: fixed duplicating panels in firefox
([hoxyq](https://github.com/hoxyq) in
[#27320](#27320))
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…ing if react is loaded (facebook#27323)

`chrome.devtools.inspectedWindow.eval` is asynchronous, so using it in
`setInterval` is a mistake.
Sometimes this results into mounting React DevTools twice, and user sees
errors about duplicated fibers in store.

With these changes, `executeIfReactHasLoaded` executed recursively with
a threshold (in case if page doesn't have react).

Although we minimize the risk of mounting DevTools twice here, this
approach is not the best way to have this problem solved. Dumping some
thoughts and ideas that I've tried, but which are out of the scope for
this release, because they can be too risky and time-consuming.
Potential changes:
- Have 2 content scripts:
  - One `prepareInjection` to notify service worker on renderer attached
- One which runs on `document_idle` to finalize check, in case if there
is no react
- Service worker will notify devtools page that it is ready to mount
React DevTools panels or should show that there is no React to be found
- Extension port from devtools page should be persistent and connected
when `main.js` is executed
- Might require refactoring the logic of how we connect devtools and
proxy ports
  
  
Some corner cases:
- Navigating to restricted pages, like `chrome://<something>` and back
- When react is lazily loaded, like in an attached iframe, or just
opened modal
- In-tab navigation with pre-cached pages, I think only Chrome does it
- Firefox is still on manifest v2 and it doesn't allow running content
scripts in ExecutionWorld.MAIN, so it requires a different approach
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
This is a patch version to fix some bugs in a previous internal release.
I am expecting this one also to be internal-only, need to make sure that
extension is stable in Chrome. Some changes and improvements are
expected for Firefox, though, before going public.

* refactor[devtools/extension]: handle ports disconnection, instead of
frequent reconnection ([hoxyq](https://github.com/hoxyq) in
[facebook#27336](facebook#27336))
* refactor[devtools/extension]: migrate from using setInterval for
polling if react is loaded ([hoxyq](https://github.com/hoxyq) in
[facebook#27323](facebook#27323))
* fix[devtools/extension]: fixed duplicating panels in firefox
([hoxyq](https://github.com/hoxyq) in
[facebook#27320](facebook#27320))
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…ing if react is loaded (#27323)

`chrome.devtools.inspectedWindow.eval` is asynchronous, so using it in
`setInterval` is a mistake.
Sometimes this results into mounting React DevTools twice, and user sees
errors about duplicated fibers in store.

With these changes, `executeIfReactHasLoaded` executed recursively with
a threshold (in case if page doesn't have react).

Although we minimize the risk of mounting DevTools twice here, this
approach is not the best way to have this problem solved. Dumping some
thoughts and ideas that I've tried, but which are out of the scope for
this release, because they can be too risky and time-consuming.
Potential changes:
- Have 2 content scripts:
  - One `prepareInjection` to notify service worker on renderer attached
- One which runs on `document_idle` to finalize check, in case if there
is no react
- Service worker will notify devtools page that it is ready to mount
React DevTools panels or should show that there is no React to be found
- Extension port from devtools page should be persistent and connected
when `main.js` is executed
- Might require refactoring the logic of how we connect devtools and
proxy ports

Some corner cases:
- Navigating to restricted pages, like `chrome://<something>` and back
- When react is lazily loaded, like in an attached iframe, or just
opened modal
- In-tab navigation with pre-cached pages, I think only Chrome does it
- Firefox is still on manifest v2 and it doesn't allow running content
scripts in ExecutionWorld.MAIN, so it requires a different approach

DiffTrain build for commit 9b4f847.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants