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: Improve browser extension iframe support #18945

Open
dmail opened this issue May 18, 2020 · 31 comments
Open

DevTools: Improve browser extension iframe support #18945

dmail opened this issue May 18, 2020 · 31 comments

Comments

@dmail
Copy link
Contributor

@dmail dmail commented May 18, 2020

When react is inside an iframe, chrome extension for react devtools fails to detect react.

This is because the extension sets __REACT_DEVTOOLS_GLOBAL_HOOK__ only on the top level window. Apparently it's possible to have __REACT_DEVTOOLS_GLOBAL_HOOK__ on iframes too by adding all_frames: true in extension manifest.json. It was done by redux devtools extension in zalmoxisus/redux-devtools-extension#56.

Have you considered adding all_frames: true to chrome extension ?

Steps To Reproduce

  1. Create a file react-main.html
<!DOCTYPE html>
<html>
  <head>
    <script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
    <script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
  </head>
  <body></body>
</html>
  1. Create a file react-iframe.html
<!DOCTYPE html>
<html>
  <head></head>
  <body>
    <iframe src="./react-main.html" />
  </body>
</html>
  1. Open react-iframe.html in chrome

The current behavior

react-devtools-not-detected

The expected behavior

react-devtools-detected

Pull request: #18952

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented May 18, 2020

The frame limitation is mentioned in the docs, FWIW:
https://github.com/facebook/react/tree/master/packages/react-devtools

It's not a bug, just a known limitation. We currently suggest using the standalone version (as linked above) or (if you also control the iframe) passing the hook through from the parent:

// Enable DevTools to inspect React inside of an <iframe>
// This must run before React is loaded
__REACT_DEVTOOLS_GLOBAL_HOOK__ = parent.__REACT_DEVTOOLS_GLOBAL_HOOK__;

I don't think that adding the Manifest setting you mentioned would be enough on its own to make the React DevTools extension detect and support versions of React running inside of iframes.

I'd be happy for you to take a shot at implementing it though if you'd like. We have a regression test that you could use to verify this:

./fixtures/devtools/regression/server.js
# Open localhost:3000 and see if DevTools detects React
@bvaughn bvaughn changed the title Bug: Chrome react devtools extension does not detect react in iframe DevTools: Improve browser extension iframe support May 18, 2020
@dmail
Copy link
Contributor Author

@dmail dmail commented May 18, 2020

Thank you very much for your answer and rewording the issue title, it's more accurate.

In my case the iframe runs in an other domain so window.parent would not be accessible.

I am ready to try to implement it. I'll take a look tomorrow. If you have any more info to share that would greatly help me when I'll start working on it.

Thanks again, I will keep you informed and I hope I will be able to make it work :)

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented May 18, 2020

Great! Keep me posted on your progress 😄

Here's instructions to get you started:
https://github.com/facebook/react/tree/master/packages/react-devtools-extensions#build-steps

@dmail
Copy link
Contributor Author

@dmail dmail commented May 19, 2020

I have opened a draft pull request.
For now I have only tested to add all_frames: true on chrome extension manifest.json and it looks like it's working -> react is being detected inside a sandboxed iframe 🎉 .

@dmail
Copy link
Contributor Author

@dmail dmail commented May 19, 2020

Working on edge 🎉

About firefox however I have some trouble. I have changed nothing yet concerning firefox and after doing

  1. yarn build:firefox
  2. yarn run test:firefox

In the firefox browser that is launched the extension does not detect react on reactjs.org.

firefoxko

In Firefox console I see this error (and many more with the same error code):

image

The extension in firefox store is working properly

firefoxok

Firefox fails to load some file (apparently related to Localization) when the extension is loaded locally. Any idea what is going on ?

@dmail
Copy link
Contributor Author

@dmail dmail commented May 19, 2020

About firefox I got workarounds depending what happens:

(a) Firefox addon is shown but fails to detect react
-> refresh the page and it will work normally

(b) Firefox addon does not even show up
-> go to Firefox settings
-> disable react devtools addon and enable it right away
-> go back to reactjs.org and it will work normally

As I said this happens already on master branch without any change on my side.

Now that I can test firefox extension locally I move on and try to add all_frame: true on Firefox.

@dmail
Copy link
Contributor Author

@dmail dmail commented May 19, 2020

I'm putting pull request in ready for review.

@shakhbulatgaz
Copy link

@shakhbulatgaz shakhbulatgaz commented Jun 25, 2020

What happened to this issue? Is it available for me to try out?
@dmail , are you planning on working on it or I may try to figure this out?

@dmail
Copy link
Contributor Author

@dmail dmail commented Jun 25, 2020

Hello, the pull request is on hold because I don't know what to tackle first. On my side I was waiting for advice to restart working on it. Go ahead, I'm still interested on the subject so I'll keep an eye on it (and may still help to the extent of my abilities).

@shakhbulatgaz
Copy link

@shakhbulatgaz shakhbulatgaz commented Jun 25, 2020

@dmail thank you!

@markerikson
Copy link

@markerikson markerikson commented Jul 2, 2020

For the record, I was able to make this work with a Next.js app that is providing the iframe, with these changes:

$NEXT_PROJECT/public/js/enableReactDevtoolsIframe.js

// The React DevTools do not normally look inside of iframes, just the outer window.
// We can enable inspection of React components in an iframe by copying this global variable:
// https://github.com/facebook/react/issues/18945#issuecomment-630421386
// This code must be injected before React runs, so we add it as a separate tag
// and show it via Next.js <Head> in _app.tsx.
if (window.parent) {
  window.__REACT_DEVTOOLS_GLOBAL_HOOK__ = window.parent.__REACT_DEVTOOLS_GLOBAL_HOOK__;
}

$NEXT_PROJECT/pages/_app.tsx

import React from 'react';
import Head from 'next/head';

export default function MyApp({ Component, pageProps }: { Component: any; pageProps: any }) {
  // Ensure the React DevTools global variable is injected if this is an iframe
  // to enable inspection of components inside the iframe.
  return (
    <React.Fragment>
      <Head>
        <script src="js/enableReactDevtoolsIframe.js"></script>
      </Head>
      <Component {...pageProps} />
    </React.Fragment>
  );
}
@marvinhagemeister
Copy link
Contributor

@marvinhagemeister marvinhagemeister commented Jul 3, 2020

FYI: We've been using the all_frames: true option in manifest.json for a while now over with preact-devtools and it works like a charm. The only thing one needs to be careful about is to inject the highlighting code into each iframe too! Otherwise the position will be off.

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 6, 2020

@marvinhagemeister Do you remember doing anything additional (beyond just adding all_frames: true option in manifest.json) to get it to work? I haven't taken the time to dig into this yet but I did test out the linked PR (#18952) and it didn't appear to be working.

@shakhbulatgaz did you ever end up digging into this?

@marvinhagemeister
Copy link
Contributor

@marvinhagemeister marvinhagemeister commented Jul 6, 2020

@bvaughn Not much really, only had to ensure that each adapter that's injected into the client doesn't traverse into other iframes. The linked PR looks correct to me, so I'm guessing something isn't ok with the connection logic. Maybe it's the same timing issue we've been talking about a while back that's more of a regression in Chrome?

For reference: The PR that added support for iframes for preact-devtools: https://github.com/preactjs/preact-devtools/pull/209/files

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 6, 2020

Gotcha! Thanks for elaborating 😄

Maybe it's the same timing issue we've been talking about a while back that's more of a regression in Chrome?

I think the fix for this issue has made its way into stable (or at least I'm no longer seeing the issue myself).

@shakhbulatgaz
Copy link

@shakhbulatgaz shakhbulatgaz commented Jul 6, 2020

@bvaughn Nah, I'm sorry, other things came up and I completely forgot about this 😢

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 6, 2020

Fair enough. Maybe someone else will pick it up :)

@anrao91
Copy link

@anrao91 anrao91 commented Jul 6, 2020

@bvaughn Can you explain to me the current scenario and what needs to be done? Was reading through the thread but couldn't deduce much. This is my first issue to pick from react repo.

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 6, 2020

@anrao91 Unfortunately, no. Not beyond what's been discussed on this thread and on the linked PR (#18952).

@Reflex-Gravity
Copy link

@Reflex-Gravity Reflex-Gravity commented Jul 13, 2020

@bvaughn By adding the below code, doesn't enable react to inspect inside an iframe. I tried the regression test, didn't work there either.

__REACT_DEVTOOLS_GLOBAL_HOOK__ = parent.__REACT_DEVTOOLS_GLOBAL_HOOK__; 

I was able to make this work by copying the values of parents __REACT_DEVTOOLS_GLOBAL_HOOK__ one by one, instead of copying it directly. (Maybe this isn't the right approach, but it seems to work)

for (const key in __REACT_DEVTOOLS_GLOBAL_HOOK__) {
      __REACT_DEVTOOLS_GLOBAL_HOOK__[key] = parent.__REACT_DEVTOOLS_GLOBAL_HOOK__[key]
}

I verified it by doing the regression test on Firefox and Chrome. It detected React and also displayed the entire component tree under the Components Panel. (Didn't work for React 0.14.9 though due to incompatibility)

Copying it directly didn't update the iframe's __REACT_DEVTOOLS_GLOBAL_HOOK__ value, I'm not really sure why though. Maybe because it's is not writable. When using defineProperty, writable is false by default. But, In this case, the above working code shouldn't have worked. 😄

Object.defineProperty(
target,
'__REACT_DEVTOOLS_GLOBAL_HOOK__',
({
// This property needs to be configurable for the test environment,
// else we won't be able to delete and recreate it beween tests.
configurable: __DEV__,
enumerable: false,
get() {
return hook;
},
}: Object),
);

Anyways let me know your thoughts on this.

Below is a sceenshot showing the working eg:,

react_dev_tools_issue

@sarathps93
Copy link

@sarathps93 sarathps93 commented Aug 30, 2020

Can I take this up if no one else is currently working on it? @bvaughn @dmail

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 31, 2020

@sarathps93 You're welcome to work on this. Please check out the work and discussion on #19345 before starting though!

@omarsy
Copy link
Contributor

@omarsy omarsy commented Sep 7, 2020

@sarathps93 Are you still working on it ?

@sarathps93
Copy link

@sarathps93 sarathps93 commented Sep 8, 2020

@omarsy I was about to start this weekend. Let me know if you want to take it up

@omarsy
Copy link
Contributor

@omarsy omarsy commented Sep 11, 2020

Can I push a PR ?

@sarathps93
Copy link

@sarathps93 sarathps93 commented Sep 13, 2020

@omarsy Yes please go ahead

@omarsy
Copy link
Contributor

@omarsy omarsy commented Sep 13, 2020

Thank you @sarathps93

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 14, 2020

Resolved in #19827

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 5, 2020

Re-opening because this was reverted again.

@bvaughn bvaughn reopened this Oct 5, 2020
@Eric0926
Copy link

@Eric0926 Eric0926 commented Oct 6, 2020

@bvaughn I have walked through previous discussions. Can I take a look at it as my first issue?

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Oct 6, 2020

Feel free, @Eric0926.

I'm a little skeptical about landing this change again though, given we've had to revert it twice. 😅 But if you're willing to do a lot of testing, sure!

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

Successfully merging a pull request may close this issue.

10 participants
You can’t perform that action at this time.