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

[DevTools] upgrade to Manifest V3 #25145

Merged
merged 12 commits into from
Oct 22, 2022
Merged

[DevTools] upgrade to Manifest V3 #25145

merged 12 commits into from
Oct 22, 2022

Conversation

mondaychen
Copy link
Contributor

@mondaychen mondaychen commented Aug 25, 2022

Summary

resolves #24522

To upgrade to Manifest V3, one of the biggest issue is that we are no longer allowed to add a script element with code in textContent so that it would run synchronously. It's necessary for us because we need to inject a global hook for react reconciler to detect whether devtools exist.
To do that, we'll leverage a new API chrome.scripting.registerContentScripts in V3. Particularly, we rely on the "world" option (added in Chrome v102 commit) to run it in the "main world" on the page.

This PR also renames a few content script files so that it's easier to tell them apart from other extension scripts and understand the purpose of each of them.

Manifest V3 is not yet ready for Firefox, so we need to keep some code for compatibility.

How did you test this change?

yarn build:chrome && yarn test:chrome
yarn build:edge && yarn test:edge
yarn build:firefox && yarn test:firefox

@mondaychen mondaychen changed the title [DevTools] upgrade Manifest V3 [DevTools] upgrade to Manifest V3 Aug 25, 2022
@SijaanX
Copy link

SijaanX commented Aug 25, 2022

@mondaychen Too bad your code looks exactly like mine, but I forgot to delete the XHR request which isn't supported.
I feel like I missed this one badly, worked on it for over a month to understand extension :(.

URGHHHH

@SijaanX
Copy link

SijaanX commented Aug 25, 2022

@mondaychen I have only a slight difference, with the injected install hook.

I made a new file as you did, but I copied the whole script (including the installhook(window)) and pasted it into the file.

You imported it from import {installHook} from 'react-devtools-shared/src/hook';. Which way is better?

I did it this way because I wanted to keep it independent of other repo's. Please let me know wha tyou think.

Again, I feel like I missed this one badly@@@@

@mondaychen
Copy link
Contributor Author

mondaychen commented Aug 26, 2022

@SijaanX

I did it this way because I wanted to keep it independent of other repo's

react-devtools-shared/src/hook is also used in other places such as standalone app and inline version of devtools, and most of the logic needs to be shared. It's better to keep it there and import from it.

I understand and appreciate that you'd love to contribute. But we have a hard deadline and cannot keep waiting. Like I've suggested in the issue, if you did make significant progress but want my help, it's better to create a branch on your own fork so that I can see it, and communicate with me clearly and timely. In that way, even if you could not finish the entire thing on your own, as long as I were able to finish the work on top of yours, you will still get credits.
Also in the future I'd suggest you work on something not assigned to a team member. It's way easier when you are not "racing" with a developer who has more context and is working on this project full-time.

@mondaychen
Copy link
Contributor Author

@lunaruan updated!

@lunaruan
Copy link
Contributor

lunaruan commented Oct 3, 2022

Sorry for the belated review! This looks good. One thing though: We don't want React DevTools to slow down the initial page load. Could we add a before/after profiling trace to make sure that the page load time has not changed?

@mondaychen
Copy link
Contributor Author

Performance impact (tested with cached reactjs.org, Chrome perf tab "start profiling & reload")

With MV3 (this PR): about 700ms
image

With MV2 (main branch): about 840ms
image

With no extension installed: about 690ms
image

My test perf results on other pages are similar. My conclusion is that this change brings improvement on initial page load time.

Copy link
Contributor

@lunaruan lunaruan left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few comments/nits!

packages/react-devtools-extensions/chrome/manifest.json Outdated Show resolved Hide resolved
packages/react-devtools/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lunaruan lunaruan left a comment

Choose a reason for hiding this comment

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

A couple of nits but looks good! Just to verify, you tested on Edge, Chrome, and Firefox right?

packages/react-devtools-extensions/firefox/manifest.json Outdated Show resolved Hide resolved
@mondaychen mondaychen merged commit 6dbccb9 into facebook:main Oct 22, 2022
@mondaychen
Copy link
Contributor Author

@lunaruan of course!

@mondaychen mondaychen deleted the mf_v3 branch October 26, 2022 21:22
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
## Summary

resolves #24522

To upgrade to Manifest V3, one of the biggest issue is that we are no
longer allowed to add a script element with code in textContent so that
it would run synchronously. It's necessary for us because we need to
inject a global hook for react reconciler to detect whether devtools
exist.
To do that, we'll leverage a new API
`chrome.scripting.registerContentScripts` in V3. Particularly, we rely
on the "world" option (added in Chrome v102
[commit](https://chromium.googlesource.com/chromium/src/+/e5ad3451c17b21341b0b9019b074801c44c92c9f))
to run it in the "main world" on the page.

This PR also renames a few content script files so that it's easier to
tell them apart from other extension scripts and understand the purpose
of each of them.

Manifest V3 is not yet ready for Firefox, so we need to keep some code
for compatibility.

## How did you test this change?

`yarn build:chrome && yarn test:chrome`
`yarn build:edge && yarn test:edge`
`yarn build:firefox && yarn test:firefox`
const ports = {};

const IS_FIREFOX = navigator.userAgent.indexOf('Firefox') >= 0;
if (!IS_FIREFOX) {
// Manifest V3 method of injecting content scripts (not yet supported in Firefox)

Choose a reason for hiding this comment

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

Hi. Not sure if this is the best place to mention it but this is now supported in Firefox and, in fact, feature detection works generally better than user-agent detection. This extension should be trivial to "port" on Firefox MV3 ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting me know! Unfortunately we are extremely understaffed now, so we won't be able to improve it soon. I'll make it as a TODO and hope we'll find time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DevTools] Manifest version 2 is deprecated
5 participants