-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
[DevTools] use backend manager to support multiple backends in extension #26615
[DevTools] use backend manager to support multiple backends in extension #26615
Conversation
// TODO: add equivalent logic for Firefox is in prepareInjection.js | ||
chrome.scripting.executeScript({ | ||
target: {tabId: id}, | ||
files: [`/build/react_devtools_backend_${version}.js`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will we keep this format of the file names?
I am not sure if this is possible, but maybe we should add an extra check here that file /build/react_devtools_backend_${version}.js
is present, log error otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. We can try-catch and log. Our error logging only works on the internal build though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will we keep this format of the file names?
For now there is only one file. In the future I will create an automated script to take care of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it is currently not possible to establish the connection to the logger in this file :(
} | ||
// only attach if the renderer is compatible with the current version of the backend | ||
if (!isMatchingRender(renderer.reconcilerVersion || renderer.version)) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this emit the unsupported-renderer-version
on line 103?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, unsupported-renderer-version
is meant to detect React older than v15. See UnsupportedVersionDialog.js
This new devtools backend will only match one version (with exception of react-devtools-backend-compact that supports v15~v18). When a match is not found, it should remain silent because another backend might be loaded.
This is a good point I should update some event names / variables / comments to increase code readability
Full list of changes: * Use .slice() for all substring-ing ([sophiebits](https://github.com/sophiebits) in [#26677](#26677)) * cleanup[devtools]: remove named hooks & profiler changed hook indices feature flags ([hoxyq](https://github.com/hoxyq) in [#26635](#26635)) * chore[devtools/release-scripts]: update messages / fixed npm view com… ([hoxyq](https://github.com/hoxyq) in [#26660](#26660)) * (patch)[DevTools] bug fix: backend injection logic not working for undocked devtools window ([mondaychen](https://github.com/mondaychen) in [#26665](#26665)) * use backend manager to support multiple backends in extension ([mondaychen](https://github.com/mondaychen) in [#26615](#26615))
…ion (#26615) In the extension, currently we do the following: 1. check whether there's at least one React renderer on the page 2. if yes, load the backend to the page 3. initialize the backend To support multiple versions of backends, we are changing it to: 1. check the versions of React renders on the page 2. load corresponding React DevTools backends that are shipped with the extension; if they are not contained (usually prod builds of prereleases), show a UI to allow users to load them from UI 3. initialize each of the backends To enable this workflow, a backend will ignore React renderers that does not match its version This PR adds a new file "backendManager" in the extension for this purpose. ------ I've tested it on Chrome, Edge and Firefox extensions
Full list of changes: * Use .slice() for all substring-ing ([sophiebits](https://github.com/sophiebits) in [#26677](#26677)) * cleanup[devtools]: remove named hooks & profiler changed hook indices feature flags ([hoxyq](https://github.com/hoxyq) in [#26635](#26635)) * chore[devtools/release-scripts]: update messages / fixed npm view com… ([hoxyq](https://github.com/hoxyq) in [#26660](#26660)) * (patch)[DevTools] bug fix: backend injection logic not working for undocked devtools window ([mondaychen](https://github.com/mondaychen) in [#26665](#26665)) * use backend manager to support multiple backends in extension ([mondaychen](https://github.com/mondaychen) in [#26615](#26615))
## 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.
Summary: ## 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/react#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 facebook/react@377c517. Changelog: [Internal] << DO NOT EDIT BELOW THIS LINE >> Reviewed By: sammy-SC Differential Revision: D45573294 Pulled By: tyao1 fbshipit-source-id: 42a8e4a54cd367080fdb7e1c357c48ae3564b7f0
Summary: ## 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/react#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 facebook/react@377c517. Changelog: [Internal] << DO NOT EDIT BELOW THIS LINE >> Reviewed By: sammy-SC Differential Revision: D45573294 Pulled By: tyao1 fbshipit-source-id: 42a8e4a54cd367080fdb7e1c357c48ae3564b7f0
…ion (facebook#26615) In the extension, currently we do the following: 1. check whether there's at least one React renderer on the page 2. if yes, load the backend to the page 3. initialize the backend To support multiple versions of backends, we are changing it to: 1. check the versions of React renders on the page 2. load corresponding React DevTools backends that are shipped with the extension; if they are not contained (usually prod builds of prereleases), show a UI to allow users to load them from UI 3. initialize each of the backends To enable this workflow, a backend will ignore React renderers that does not match its version This PR adds a new file "backendManager" in the extension for this purpose. ------ I've tested it on Chrome, Edge and Firefox extensions
Full list of changes: * Use .slice() for all substring-ing ([sophiebits](https://github.com/sophiebits) in [facebook#26677](facebook#26677)) * cleanup[devtools]: remove named hooks & profiler changed hook indices feature flags ([hoxyq](https://github.com/hoxyq) in [facebook#26635](facebook#26635)) * chore[devtools/release-scripts]: update messages / fixed npm view com… ([hoxyq](https://github.com/hoxyq) in [facebook#26660](facebook#26660)) * (patch)[DevTools] bug fix: backend injection logic not working for undocked devtools window ([mondaychen](https://github.com/mondaychen) in [facebook#26665](facebook#26665)) * use backend manager to support multiple backends in extension ([mondaychen](https://github.com/mondaychen) in [facebook#26615](facebook#26615))
## 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.
…ion (#26615) In the extension, currently we do the following: 1. check whether there's at least one React renderer on the page 2. if yes, load the backend to the page 3. initialize the backend To support multiple versions of backends, we are changing it to: 1. check the versions of React renders on the page 2. load corresponding React DevTools backends that are shipped with the extension; if they are not contained (usually prod builds of prereleases), show a UI to allow users to load them from UI 3. initialize each of the backends To enable this workflow, a backend will ignore React renderers that does not match its version This PR adds a new file "backendManager" in the extension for this purpose. ------ I've tested it on Chrome, Edge and Firefox extensions DiffTrain build for commit d962f35.
In the extension, currently we do the following:
To support multiple versions of backends, we are changing it to:
To enable this workflow, a backend will ignore React renderers that does not match its version
This PR adds a new file "backendManager" in the extension for this purpose.
I've tested it on Chrome, Edge and Firefox extensions