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

react-refresh + ReactDOM: hot reloading only works when bundling React #17626

Closed
mxmul opened this issue Dec 16, 2019 · 7 comments · Fixed by #17633
Closed

react-refresh + ReactDOM: hot reloading only works when bundling React #17626

mxmul opened this issue Dec 16, 2019 · 7 comments · Fixed by #17633

Comments

@mxmul
Copy link

mxmul commented Dec 16, 2019

Do you want to request a feature or report a bug?

bug?

What is the current behavior?

note: I am encountering this issue when using https://github.com/pmmmwh/react-refresh-webpack-plugin, but I believe it's an issue with react-refresh itself.

The react-refresh runtime overrides __REACT_DEVTOOLS_GLOBAL_HOOK__.inject to get a reference to the React renderer. In my app, the inject method is never called however, because I load react/react-dom from a third-party CDN before my application code. This means that changed components are never actually refreshed in the DOM.

I believe the issue is that scripts are loaded in this order:

  1. react-devtools sets up the global hook
  2. react/react-dom are loaded on the page, and inject() is called
  3. user code (which is instrumented with the react-refresh babel plugin) is loaded on the page, and inject() is monkey-patched

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

Follow the usage steps in the https://github.com/pmmmwh/react-refresh-webpack-plugin repo, but also add react and react-dom as externals in your Webpack build:

module.exports = {
  //...
  externals: {
    react: 'React',
    'react-dom': 'ReactDOM'
  }
};

then load those scripts onto the page from a CDN (e.g. unpkg or cdnjs) before the Webpack bundle.

What is the expected behavior?

react-refresh works the same whether React/ReactDOM are bundled with application code, or loaded via an external script.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.8.6 / Chrome / osx

@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2019

I think one possible fix is to add some logic here:

hook.inject = function(injected) {

Currently it sets things up only for renderers that inject later. But we can probably refactor this in a way that could also do something useful for already injected renderers. At least for the case where no roots are mounted yet (which would be your use case), this seems like it shouldn't be a lot of work.

Do you want to take this?

@mxmul
Copy link
Author

mxmul commented Dec 17, 2019

sure, I'll work on this

@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2019

Actually I think I might have a solution. Sorry for the churn! There are some nuances so it's probably going to be faster if I just push a PR.

@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2019

Can you check if #17633 helps?

@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2019

Let's track this in existing issue: #17552.

@gaearon gaearon closed this as completed Dec 17, 2019
@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2019

Fixed in [email protected]. You'll need a development version of React and >= 16.10 or so.

@benjamingr
Copy link

I have a weird ask - is there a way for react-refresh to work with a production version of React? (and load after)

Our current workaround (as a plugin hosted in another site) is to use a browser extension to swap the react production build for react development build but that's not really optimal.

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

Successfully merging a pull request may close this issue.

3 participants