-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
[Fast Refresh] Support injecting runtime after renderer executes #17633
Conversation
@@ -435,6 +435,7 @@ export function injectIntoGlobalHook(globalObject: any): void { | |||
// Otherwise, the renderer will think that there is no global hook, and won't do the injection. | |||
let nextID = 0; | |||
globalObject.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook = { | |||
renderers: new Map(), |
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.
This is just so the lines below don't crash when we take this code branch. This particular Map isn't actually used further.
@@ -468,6 +469,19 @@ export function injectIntoGlobalHook(globalObject: any): void { | |||
return id; | |||
}; | |||
|
|||
// Do the same for any already injected roots. |
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.
Note this is almost copy paste from the code above (462–468). I could extract a function but meh.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 43c8eff:
|
Details of bundled changes.Comparing: 7c21bf7...43c8eff react-refresh
Size changes (stable) |
Details of bundled changes.Comparing: 7c21bf7...43c8eff react-refresh
Size changes (experimental) |
I can't get it to work unless I enable React Development Tools, is this expected behavior? Not a big deal but I can't seem to get it to do it's magic unless React Development Tools is runnig. |
Sorry, not enough details. Please file a new issue. |
I think this fixes #17626 and #17552.
I was assuming that Fresh runtime always executes before the renderer. But on the web this won't be the case if you load React from CDN and then run your webpack bundle (pmmmwh/react-refresh-webpack-plugin#13). And separating
<script>
tags can be annoying and complicated.Instead, we can handle the simple case — when ReactDOM executes before the webpack bundle — gracefully. The DevTools global hook already has a renderers map so all we need to connect to them is to read from it. In fact that's how DevTools backends themselves connect to already injected renderers.
This still doesn't handle the case where you mount some roots before injecting the runtime. In that case they won't be "visible" to Fresh. Arguably this is more of a corner case and I wouldn't bother solving this yet. The webpack plugin should be able to ensure that injection happens before the actual app code runs.
I added a regression test.