Skip to content

[Fresh] Track mounted roots via DevTools Hook#15928

Merged
gaearon merged 4 commits into
facebook:masterfrom
gaearon:fresh-dt
Jun 19, 2019
Merged

[Fresh] Track mounted roots via DevTools Hook#15928
gaearon merged 4 commits into
facebook:masterfrom
gaearon:fresh-dt

Conversation

@gaearon
Copy link
Copy Markdown
Collaborator

@gaearon gaearon commented Jun 19, 2019

The previous Fresh runtime API forced you to pass a root handle. However, that presumes that the module runtime (such as Metro) somehow is aware of all roots on the page. This isn't the case.

We could have React keep track of mounted roots. However, that might not be necessary. Here's why.

We still need some way to let the module runtime schedule refreshes. Currently this is done via DevTools Global Hook injection. DevTools (or module runtime) can set up the Global Hook. The renderer injects scheduleHotUpdate in it. As a result, the module runtime can schedule refreshes.

Since we already rely on the DevTools Hook for communication, we might as well use the fact that this Hook gives us information about all roots. Indeed, that's how DevTools itself works.

The Goal

This PR lets the module runtime do

ReactRefreshRuntime.injectIntoGlobalHook(window);

and then

ReactRefreshRuntime.performReactRefresh(update);

without worrying about how to track mounted roots.

Implementation

Adding injectIntoGlobalHook to Refresh Runtime

  • This either sets up a shimmed tiny version of the Global Hook (for web users without extension).
  • Or, if the Global Hook is already set up (for web users with extension, or for RN which always sets it up), monkeypatches it.

The goal in both cases is to grab the scheduling functionality and to track roots in the Runtime. This lets the module system (e.g. Metro) simply call ReactRefreshRuntime.performReactRefresh() and not worry about how this connects to React.

The only constraint is that ReactRefreshRuntime.injectIntoGlobalHook() needs to run before the renderer code. The module system can guarantee it because it owns the execution order.

I have added regression tests for tracking roots.

Tweaking the API exposed to DevTools

Previously, we had scheduleHotUpdate(root, update) which included the resolution function. However, I realized this doesn't really work if you don't have a root. For example, if you hot update a component, but no roots are mounted yet, you still want its next render to use the updated version. So you need to inject the resolution function anyway.

I changed the API to:

  • setRefreshHandler(handler) — This sets up the family resolution function. It is called on first edit even if there are no roots.
  • scheduleRefresh(root, update) — renamed from scheduleHotReload
  • findHostInstancesForRefresh — renamed from findHostInstancesForHotReload

Follow-ups

There's a few things I plan to change here later. One is to use host config for getting client rect. But this isn't critical to get in now, since I don't plan to implement this in the first Metro integration. The second one is to add a few convenience helpers to move more React-related logic out of the module runtime (e.g. Metro). I might push that a bit later but it doesn't block this PR.

@sizebot
Copy link
Copy Markdown

sizebot commented Jun 19, 2019

Details of bundled changes.

Comparing: 9845437...d20de50

react-refresh

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-refresh-runtime.development.js +146.2% +113.8% 6.8 KB 16.74 KB 2.46 KB 5.25 KB NODE_DEV
react-refresh-runtime.production.min.js -27.3% -55.4% 1.93 KB 1.4 KB 1009 B 450 B NODE_PROD
react-refresh-babel.development.js 0.0% -0.1% 21.67 KB 21.67 KB 4.65 KB 4.65 KB NODE_DEV
react-refresh-babel.production.min.js 0.0% -0.2% 6.3 KB 6.3 KB 2.17 KB 2.16 KB NODE_PROD

Generated by 🚫 dangerJS

@gaearon
Copy link
Copy Markdown
Collaborator Author

gaearon commented Jun 19, 2019

Pushed a second commit. This just adds some helpers that are currently inlined in Metro in my draft. It doesn't make sense to inline them in Metro because they're more React-specific. And other integrations will need the same helpers. So I put them in the runtime too.

These utilities will likely be needed by all module systems, so let's just put them here.
@gaearon
Copy link
Copy Markdown
Collaborator Author

gaearon commented Jun 19, 2019

Verified this works in RN.

// Note that in this case it's important that renderer code runs *after* this method call.
// Otherwise, the renderer will think that there is no global hook, and won't do the injection.
globalObject.__REACT_DEVTOOLS_GLOBAL_HOOK__ = hook = {
supportsFiber: true,
Copy link
Copy Markdown
Contributor

@bvaughn bvaughn Jun 19, 2019

Choose a reason for hiding this comment

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

We should eventually remove this check in React 😆

@bvaughn
Copy link
Copy Markdown
Contributor

bvaughn commented Jun 19, 2019

Changes seem good. Your recent prod-only errors are causing test-prod tests to fail though 😁

@gaearon gaearon merged commit e9d0a3f into facebook:master Jun 19, 2019
@gaearon gaearon deleted the fresh-dt branch June 19, 2019 23:12
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 21, 2019
Summary:
This pulls in the latest package updates for Fresh. It doesn't have any user-observable behavior.

The renderer is rebuilt on top of the last cherry-picked sync. I cherry-picked facebook/react#15928 on top of it.

Reviewed By: rickhanlonii

Differential Revision: D15901887

fbshipit-source-id: ccd974f79e4c0a2a8a8cab0d472deeaedf1e3ddd
gaearon added a commit to gaearon/react that referenced this pull request Jun 24, 2019
* Track mounted roots via DevTools Hook

* Add helper utilities to the runtime

These utilities will likely be needed by all module systems, so let's just put them here.

* Wrap more things in __DEV__

* Fix tests to also be DEV-only
rickhanlonii pushed a commit to rickhanlonii/react that referenced this pull request Jun 25, 2019
* Track mounted roots via DevTools Hook

* Add helper utilities to the runtime

These utilities will likely be needed by all module systems, so let's just put them here.

* Wrap more things in __DEV__

* Fix tests to also be DEV-only
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
This pulls in the latest package updates for Fresh. It doesn't have any user-observable behavior.

The renderer is rebuilt on top of the last cherry-picked sync. I cherry-picked facebook/react#15928 on top of it.

Reviewed By: rickhanlonii

Differential Revision: D15901887

fbshipit-source-id: ccd974f79e4c0a2a8a8cab0d472deeaedf1e3ddd
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.

4 participants