Suggested code improvements and feature requests in chrome extension: 1. Inject injectGlobalHook.js only on click of extension icon or atleast use document_idle and 2. handle protected chrome urls properly 3. Improve support for multiple iframes(show seperately relevant info for each if React detected) 4. Add unit and integration tests for the extension

[sktguha]

1. Code improvements:
   Edit 2: My apologies, I think point 1 is too much work for too little benefit. I don't think it is needed to be done(maybe we can do the quickfix as mentioned in Edit 1 below), not sure fully however. But please take a look at the other points 2,3 and 4
   
   Edit 1: As a quick fix, maybe we can put in the extension description to the user that if they wish to, they can restrict the extension to work only on click(it can be done from chrome itself now by right clicking on the extension icon) . But I still feel the changes below might be a good idea as it is sub-optimal UX, as they need to reload the page again on clicking the icon if they have made the change of work only on click by right clicking on the extension icon(best if the extension does it natively) and also from a security standpoint, as all users don't know about this or may not have done this change
   
   
   So in the chrome extension manifest.json https://github.com/facebook/react/blob/4ead6b53057ee6c6129a6d2f6e264232130b1fce/packages/react-devtools-extensions/chrome/manifest.json#L56, the injectGlobalHook.js gets injected at document_start, which I think is not the best way.
   
   Instead, I suggest we should inject and execute the script when the extension icon is clicked, as it is not a good idea to inject code on every website the user loads as it slows down the website unnecessarily and also some users may feel their privacy is violated.
   
   Also if some security vulnerability is there then it will compromise all websites the user has visited( on a very cynical note, what if tomorrow a vulnerability is discovered in the injectGlobalHook or content-script and an idiot journalist accuses fb of deliberately putting that code in 2million+ users' browser. To be very clear, I am not saying that or suggesting that in any form, it is just a worst-case scenario that can arise, in my opinion ). The backend.js code being very small, the likelihood of security vulnerability there is much smaller and it's much easier to secure. If we have to ability to inject the script on demand, we should do that.
   
   Or if we are not able to do that for some reason, at least we should set run_atto document_idle instead of document_start (we can omit the run_at maybe as it defaults to document_idle). Basically chrome will try to minimize the penalty in loading time of the web page if we use document_idle
   
   Also if we need to run the script at start, as the devtools pane components need the information, we can detect the components devtools pane onload and send a message to the background page and thus inject the script at that time, similar to the suggested inject on click on icon mechanism above. If there is a concern with the delay that will be introduced by this load on demand, maybe we can have it opt_in, via preferences, where user can choose whether to go with the slower load on demand( I am not even sure that there will be any noticeable delay, haven't done any tests) or with the fast one, but all websites will have the content_script and the injectGlobalHook loaded.

2. Feature request: It should handle protected chrome urls (like chrome:// and https://chrome.google.com/webstore ) differently. Currently it shows the generic message "This page doesn’t appear to be using React." . This gives a wrong idea to the user. Instead we should show something like "React devtools cannot run on protected chrome pages" and probably update the relevant readme section as well.

3. Feature request: In case of multiple iframes , like in codesandbox we have iframes running prod builds and dev builds, the extenson should clearly show the list of iframes and the version detected for each one. And maybe the relevant popup for unminified, dead code , old version etc. or it can show that page has multiple iframes but React is detected on only these and all others don't have React etc. Needs more discussion on this I feel.

4. Add unit and integration tests for the extension: I could be wrong, like maybe tests already exist, but I think we need to have some unit tests and integration tests for the extension. i.e we can load some websites and then test that extension is detecting them properly or not.

If any/all of these needs to be worked on, I would love to work on them. Please assign this issue to me in that case.
Thanks,
Saikat Guha(sktguha@gmail.com)

[bvaughn]

I don't think this type of issue is a great way to file feature requests. It's too broad. I'm going to close it and I recommend you re-open more focused issues if you'd like to pursue any of the individual items.

1. Inject injectGlobalHook.js only on click of extension icon or at least use document_idle

This would essentially break the extension. DevTools needs to be injected into the page before React is loaded in order for the two to talk to each other. (During initialization, React checks for a DevTools global inject method and attaches the two. Once React has been initialized– there is no more check for this.)

2. handle protected chrome urls properly

Haven't thought about this deeply but it seems like this would be a reasonable follow up issue/PR.

3. Improve support for multiple iframes(show seperately relevant info for each if React detected)

This could be filed as a separate issue but I think we would be unlikely to act on it. (In my opinion, the amount of effort seems too large for the resulting edge case improvement.)

4. Add unit and integration tests for the extension

The extension already has pretty good unit and integration tests:
https://github.com/facebook/react/tree/master/packages/react-devtools-shared/src/__tests__

[sktguha]

Thanks @bvaughn for the reply. Really sorry about creating such a large issue, but actually I created them in one issue as I didn't want to create too much noise in the repo and wanted to clear with the team before proceeding.

1. yes indeed I tried it out myself. It didn't work.

2. Yes indeed, we have already discussed this here Update outdated links and fix two broken links  #19985 (comment), I am working on a PR and within half an hour or so , will create a PR for v1 of this. I have a few follow-up questions I want to clear with you.
   EDIT: you can check current work on this branch : master...sktguha:devtools-handle-restricted-pages . need to test properly . Then will create a PR off this. however, have some follow-up questions to clear with you.

3. ok that is true. Maybe an easier approach is to at least mention in the popup and tell the user that this page has multiple iframes, if they are there. we have only detected on the main page but we haven't checked the iframes. It is not supported

4. sorry the link is coming as broken. could you please give a permalink to that? also in the readme , https://github.com/facebook/react/blob/master/packages/react-devtools-extensions/README.md , it is not mentioned how to run those unit and integration tests. The yarn run test:chrome mentioned in the readme installs the extension and simply opens an url in the chrome browser. I don't think it runs those tests that you have linked ? I could be wrong though. I didn't see any output on the terminal that tests have run and passed etc

[sktguha]

EDIT: 5. also I am not sure if

react/packages/react-devtools-extensions/popups/shared.js

Line 6 in 7e405d4

// Make links work

is required. Maybe it was required in older browser, but I removed this code, to listen to clicks and added target="_blank" in the links , and it works properly(didn't push it though yet) @bvaughn

[bvaughn]

sorry the link is coming as broken. could you please give a permalink to that? also in the readme ,

Hm, that's because GitHub is interpreting the "_" in the link as markdown I think. Here is the link:

https://github.com/facebook/react/tree/master/packages/react-devtools-shared/src/__tests__

[sktguha]

ok sure.

The yarn run test:chrome mentioned in the readme installs the extension and simply opens an url in the chrome browser. I don't think it runs those tests that you have linked ? I could be wrong though. I didn't see any output on the terminal that tests have run and passed etc

what about this ?
I am unable to find any readme in that folder, that shows how to run these tests
can't see any test script also in https://github.com/facebook/react/blob/master/packages/react-devtools-shared/package.json either