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

Event Replaying #16725

Merged
merged 12 commits into from
Sep 23, 2019
Merged

Event Replaying #16725

merged 12 commits into from
Sep 23, 2019

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Sep 10, 2019

There's a gap between when we call createRoot or createSyncRoot and when we actually commit the hydrated tree. E.g. it can take a while before the user calls root.render(). In batched mode, there's a gap before the scheduled callback actually starts rendering. In concurrent mode, there's a gap every time we yield. Additionally, for Suspense boundaries, we use Partial Hydration and there's a gap between rendering each new level.

In between these gaps, there can be events issued on dehydrated DOM nodes that are currently dropped. This adds the basic infra for replaying those events.

It is not behind a feature flag or any mode because it should not be possible to get into this mode in the legacy ReactDOM.hydrate(...) API since it doesn't leave any gaps. So if there is a reason this has any behavior changes, that's a bug and we'd like to find it.

Principle

The constraint here is that we can't wait for the actual components to load/render before showing the server rendered HTML. We also can't load much in terms of even the event system. We're limited to as much script we can reasonably inline into the HTML page which isn't much.

Some interactions don't make sense to do any other way than handling once we do have the code. Such a click on a rich UI. These needs to be prioritized.

Other interactions can't be replayed later. They have to be dealt with by the browser right now. Such as text input.

Many small interactions such as hovering or scrolling a bit also is fairly harmless.

Some events are not really user driven and can be inferred by a hydrating component (such as image "load").

There are cases where the dehydrated tree needs to be dropped and we can no longer replay that event. Such as if it gets deleted by a previous event or forced client-rendered. We could use this mechanism to force hydration even synchronously such as before invoking a discrete event. However, the issue with doing that is that there are so many discrete events that happen just by navigating around such as keypresses or touch to scroll. This would also not work before the code has loaded so we'd have to always wait to show the SSR content until the code has loaded. This wouldn't work with the plain HTML option.

My hypothesis here is that we can get far by letting the browser do most of what it does by default. Server rendered components would know to be somewhat resilient to this. Then we simply replay those events as "passive" events later to let JS observed what happened. This inherently is a somewhat degrated experience.

Event Types

I've categorized events as "discrete", "continuous" or "other".

The "discrete" category are similar to Discrete priority events. It's events that needs to be replayed in the order they were issued and each one needs to be replayed.

The "continuous" category are things like mouseover, focus, selectionchange. It's expected that there are a lot of these while the user is simply navigating the page. However, the previous ones are not really relevant and can be collapsed. Only the last state for each type needs to be replayed. There's only one "hover target" and one "active element" once we hydrate. The intermediate steps are not relevant.

The "other" category don't need to be replayed. Either because it can be replayed by the Host component hydrating itself or it's not a critical event.

In this PR:

  • Replay continuous events.
  • Listen to all replayable events eagerly when we call createRoot.
  • Tests

Follow up PRs:

  • Avoid attaching active listeners eagerly in Flare.
  • Expose callback when an event doesn't get a target during replay. I.e. it's "dropped".
  • Don't replay events if beforeunload fires. This means a link or form successfully navigated. This avoids double side-effects by the browser navigation and JS both triggering.
  • Allow grandfathered events to be passed to ReactDOM.createRoot(..., {replayEvents: ...}). This allows an early light script to track events that happened before even React loaded onto the page.
  • Increase priority of suspense boundaries that are blocked on for discrete updates to User Blocking priority. (All boundaries or just the first event?) This gives us "selective hydration".
    • We have an option to also attempt at sync priority which could allow preventDefault behavior if that succeeds. This is useful for handling links with JS.
  • Increase priority of suspense boundaries that are blocked on for continuous updates to Low priority.
  • Add API such as ReactDOM.hydrate(node) to increase the priority of that boundary to Normal pri or whatever scheduler priority context we're running in.
  • Timeout boundary if it takes too long to hydrate.

In future PRs we can improve the actual replaying mechanism based on special cases. I'm sure there's a long tail.

In particular the integration with Flare has some flaws right now. We can also evaluate having Flare replaying happen at other levels such as in the synthetic layer.

@sizebot
Copy link

sizebot commented Sep 10, 2019

Details of bundled changes.

Comparing: 70754f1...0b322c4

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +2.6% +2.5% 117.44 KB 120.51 KB 36.98 KB 37.9 KB NODE_PROFILING
ReactDOM-dev.js +1.6% +1.8% 962.16 KB 977.87 KB 212.01 KB 215.76 KB FB_WWW_DEV
react-dom-test-utils.development.js 0.0% 0.0% 57.46 KB 57.46 KB 15.82 KB 15.83 KB UMD_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.78 KB 3.78 KB 1.53 KB 1.53 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 11.19 KB 11.19 KB 4.16 KB 4.16 KB UMD_PROD
ReactDOMUnstableNativeDependencies-dev.js 0.0% +0.1% 58.95 KB 58.97 KB 14.88 KB 14.89 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-prod.js -0.2% 🔺+0.1% 26.1 KB 26.05 KB 5.27 KB 5.28 KB FB_WWW_PROD
react-dom-test-utils.development.js 0.0% 0.0% 55.73 KB 55.74 KB 15.49 KB 15.5 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% -0.0% 10.96 KB 10.96 KB 4.09 KB 4.09 KB NODE_PROD
react-dom.development.js +1.6% +1.7% 937.03 KB 951.93 KB 211.33 KB 214.82 KB UMD_DEV
react-dom.production.min.js 🔺+2.6% 🔺+2.7% 113.77 KB 116.77 KB 36.66 KB 37.64 KB UMD_PROD
react-dom.profiling.min.js +2.6% +2.6% 117.31 KB 120.32 KB 37.67 KB 38.66 KB UMD_PROFILING
react-dom.development.js +1.6% +1.7% 931.1 KB 946.02 KB 209.77 KB 213.28 KB NODE_DEV
react-dom.production.min.js 🔺+2.7% 🔺+2.5% 113.71 KB 116.77 KB 35.99 KB 36.9 KB NODE_PROD
ReactDOM-prod.js 🔺+2.8% 🔺+2.6% 382.07 KB 392.73 KB 69.76 KB 71.58 KB FB_WWW_PROD
ReactDOM-profiling.js +2.8% +2.6% 388.01 KB 398.7 KB 70.81 KB 72.63 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.development.js 0.0% +0.1% 60.71 KB 60.73 KB 15.84 KB 15.85 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js -0.0% -0.0% 10.75 KB 10.74 KB 3.67 KB 3.67 KB UMD_PROD
ReactDOMServer-dev.js 0.0% -0.0% 142.68 KB 142.68 KB 35.75 KB 35.75 KB FB_WWW_DEV
ReactDOMServer-prod.js 0.0% 0.0% 48.55 KB 48.55 KB 11.13 KB 11.13 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.39 KB 60.4 KB 15.72 KB 15.73 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 3.87 KB 3.87 KB 1.51 KB 1.51 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js -0.0% -0.0% 10.49 KB 10.49 KB 3.57 KB 3.57 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js 0.0% 0.0% 670.74 KB 671.07 KB 145.45 KB 145.49 KB UMD_DEV
react-art.production.min.js 0.0% 0.0% 103.54 KB 103.54 KB 31.55 KB 31.55 KB UMD_PROD
react-art.development.js +0.1% 0.0% 601.42 KB 601.75 KB 128.03 KB 128.09 KB NODE_DEV
ReactART-dev.js +0.1% 0.0% 617.38 KB 617.71 KB 127.84 KB 127.9 KB FB_WWW_DEV
ReactART-prod.js -0.4% -0.4% 232.08 KB 231.27 KB 39.39 KB 39.22 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js +0.1% 0.0% 628.33 KB 628.66 KB 130.24 KB 130.29 KB FB_WWW_DEV
react-test-renderer-shallow.development.js 0.0% 0.0% 39.28 KB 39.28 KB 9.97 KB 9.97 KB UMD_DEV
react-test-renderer-shallow.production.min.js 0.0% 0.0% 11.45 KB 11.45 KB 3.54 KB 3.54 KB UMD_PROD
react-test-renderer-shallow.development.js 0.0% 0.0% 33.24 KB 33.24 KB 8.51 KB 8.51 KB NODE_DEV
react-test-renderer.development.js +0.1% 0.0% 614.78 KB 615.11 KB 130.81 KB 130.87 KB UMD_DEV
react-test-renderer.production.min.js 0.0% 0.0% 70.49 KB 70.49 KB 21.63 KB 21.63 KB UMD_PROD
react-test-renderer.development.js +0.1% +0.1% 610.09 KB 610.42 KB 129.62 KB 129.68 KB NODE_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.1% 0.0% 600.11 KB 600.51 KB 126.75 KB 126.81 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.4% 🔺+0.3% 70.62 KB 70.89 KB 20.88 KB 20.96 KB NODE_PROD
react-reconciler-reflection.development.js +3.3% +2.0% 19.52 KB 20.16 KB 6.38 KB 6.5 KB NODE_DEV
react-reconciler-reflection.production.min.js 🔺+10.5% 🔺+7.7% 2.6 KB 2.88 KB 1.16 KB 1.25 KB NODE_PROD
react-reconciler-persistent.development.js +0.1% 0.0% 597.12 KB 597.52 KB 125.5 KB 125.56 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.4% 🔺+0.3% 70.63 KB 70.9 KB 20.89 KB 20.96 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-prod.js -0.0% 0.0% 275.26 KB 275.23 KB 47.13 KB 47.14 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js -0.0% -0.0% 284.65 KB 284.62 KB 48.84 KB 48.84 KB RN_OSS_PROFILING
ReactFabric-prod.js -0.0% 0.0% 267.01 KB 266.99 KB 45.7 KB 45.71 KB RN_OSS_PROD
ReactFabric-profiling.js -0.0% -0.0% 276.83 KB 276.8 KB 47.51 KB 47.51 KB RN_OSS_PROFILING
ReactFabric-dev.js +0.1% +0.1% 756.45 KB 756.96 KB 159.4 KB 159.51 KB RN_FB_DEV
ReactFabric-prod.js -0.0% 0.0% 267.02 KB 266.99 KB 45.72 KB 45.72 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.1% +0.1% 749.84 KB 750.36 KB 158.23 KB 158.33 KB RN_OSS_DEV
ReactFabric-profiling.js -0.0% -0.0% 276.82 KB 276.8 KB 47.52 KB 47.52 KB RN_FB_PROFILING
ReactNativeRenderer-dev.js +0.1% +0.1% 750 KB 750.52 KB 158.31 KB 158.41 KB RN_FB_DEV
ReactNativeRenderer-prod.js -0.0% 0.0% 275.25 KB 275.23 KB 47.14 KB 47.14 KB RN_FB_PROD
ReactNativeRenderer-profiling.js -0.0% -0.0% 284.64 KB 284.61 KB 48.85 KB 48.84 KB RN_FB_PROFILING
ReactFabric-dev.js +0.1% +0.1% 756.28 KB 756.79 KB 159.32 KB 159.42 KB RN_OSS_DEV

Generated by 🚫 dangerJS against 0b322c4

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

A few points so far (I know it's WIP):

  • It adds quite a bit of code size. We could try normalizing some of it maybe?
  • You handle different Pointer Events by pointer ID, that's good. Shouldn't we do the same for touch events and their IDs too?

@trueadm
Copy link
Contributor

trueadm commented Sep 10, 2019

In particular the integration with Flare has some flaws right now. We can also evaluate having Flare replaying happen at other levels such as in the synthetic layer.

Agreed. I have a bunch of ideas of how we might do this, but we can talk about them in our meeting later this week.

@sebmarkbage
Copy link
Collaborator Author

It adds quite a bit of code size. We could try normalizing some of it maybe?

Yea. I originally tried that but there's some quirks:

  1. The legacy system uses simple event plugin for normalizing event priorities which is actually a bug (the change event gets the wrong priority) because not all events go through simple event plugin.
  2. We need to eagerly listen to a bunch of these events which is a list that doesn't exist yet and it might be better to keep that short, since we might want that on the page before event React loads.
  3. The list is currently shared between the two event systems but ideally we wouldn't couple it to either (to avoid bloating the current bundle with Responder system, or bloating the future bundle by pulling in the legacy system).

So basically, I don't know what the easy fix is here.

@pieterv
Copy link
Member

pieterv commented Sep 10, 2019

I'm interested in how this works across suspense boundaries, is the following true?

If we receive a discrete event for boundary A (1), the for boundary B (2) then another for A (3). Once A hydrates (1) will be replayed, and (3) will be left in the queue behind (2). If a user clicks in the hydrated boundary A, the event (4) will be queued behind (3) and only re played once boundary B is unblocked.

So we could be have situations where if any one section is slow to hydrate it could switch the page back to being unresponsive if someone interacts with it? I wonder if we need the full app to have the same queue, maybe we want to allow areas to indicate they are isolated i.e. a navigation bar (that said nav events most likely could be handled via the browser) or page columns.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Sep 10, 2019

@pieterv That's correct. It's difficult to predict how the events might correlate which is why React even in concurrent mode must flush one event after another.

For the case where a nav is handled by the browser, that will interrupt the replaying even of previous events in the current approach. Meaning that even other queued events won't replay if beforeunload happens. The idea is that it's fine for user space code to call preventDefault which then won't trigger beforeunload and therefore we won't cancel replaying.

I think this should be mostly fine because if you're able to interact with two completely disconnected trees that's already very unlikely to have happened very fast since even a single interaction takes a while. We should also be reasonably fast to hydrate after the first interaction with the prioritization, and if not, then this approach kind of falls apart.

Additionally, there's an additional fail safe that I plan to add. Currently, there's no timeout associated with the replaying. Things gets blocked indefinitely because the normal expiration doesn't apply to hydration. I plan on adding a timeout when a boundary should give up. If the section for boundary B is slow to load, it'll replace it with a fallback and drop that event (replay without a target) which will then unblock the next events. My current thinking was that this would be when we're I/O bound though, not if the section is CPU bound.

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Sep 10, 2019

I do think we'll need more "features" like allowing events to preventDefault before replaying or other advanced things using some annotation in the DOM but that would be a new API on a per event level. This could also be used to opt-out of discrete ordering potentially. But that would be on a per-event basis rather than a whole subtree.

@devknoll
Copy link
Contributor

I plan on adding a timeout when a boundary should give up. If the section for boundary B is slow to load, it'll replace it with a fallback and drop that event

@sebmarkbage Have you put any thought into if/how React users will be able to customize this sort of behavior? For example, it might be more desirable to keep the dehydrated content visible but render a loading modal or something.

@sebmarkbage
Copy link
Collaborator Author

@devknoll Suspense is becoming a default mechanism for handling all kinds of scenarios like this in React so you're expected to have to have a reasonable boundary. In the case of an I/O bound timeout, it's effectively the same as attempting to render a lazy loaded component during an update and it suspending causing an update to trigger the fallback.

In that case there's no reasonable alternative since there's no way to render a consistent tree while having some it show old data. We long ago gave up on the idea of allowing partial tearing of a tree.

In theory, replaying could have an alternative such as setting state elsewhere but then that just opens the question, what if that alternate state hasn't loaded yet? Also, if you're already having to model the fallback loading state, who is actually going to design another loading state for this special edge case that really should never happen. If we're not able to replay events quickly enough in cases other than edge cases, we've really lost.

One thing we could do though is for example switch the cursor to wait if it takes a while to allow replaying to take a bit longer by giving some kind of indicator. (This is also where it would be nice if we could trigger a loading indicator in the browser where there's no cursor.)

Eventually we'll have to drop it and in that case the fallback is quite nice, because a) it already exists anyway so there's nothing extra to design and download. b) it indicates why the event was dropped. It's similar to clicking one element and then quickly another where the first one deletes the second one. It's not that confusing that the event was dropped in that scenario.

@sebmarkbage
Copy link
Collaborator Author

You handle different Pointer Events by pointer ID, that's good. Shouldn't we do the same for touch events and their IDs too?

I was originally looking at touch events but there's not really an "over" event - only moves against the started target. This doesn't track every move but only when it changes target. This use case is really more for the hover case anyway which is only modeled using pointer events.

@sebmarkbage sebmarkbage force-pushed the eventreplaying branch 5 times, most recently from c0d0678 to bdacb27 Compare September 13, 2019 21:41
@sebmarkbage sebmarkbage changed the title [WIP] Event Replaying Event Replaying Sep 13, 2019
@necolas necolas self-requested a review September 13, 2019 22:22
@sebmarkbage sebmarkbage force-pushed the eventreplaying branch 2 times, most recently from ce9d60d to c6d5872 Compare September 17, 2019 21:58
@sebmarkbage sebmarkbage force-pushed the eventreplaying branch 3 times, most recently from be1f29f to cfb0988 Compare September 23, 2019 05:25
@@ -14,3 +14,4 @@ export const RESPONDER_EVENT_SYSTEM = 1 << 1;
export const IS_PASSIVE = 1 << 2;
export const IS_ACTIVE = 1 << 3;
export const PASSIVE_NOT_SUPPORTED = 1 << 4;
export const IS_REPLAYED = 1 << 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

@@ -100,6 +101,7 @@ function _receiveRootNodeIDEvent(
batchedUpdates(function() {
runExtractedPluginEventsInBatch(
topLevelType,
RESPONDER_EVENT_SYSTEM,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we passing RESPONDER_EVENT_SYSTEM? That flag was meant for the new event system.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😯🙃Nice catch! This was an oversight. It should be passing the plugin event system flag.

@@ -313,6 +314,7 @@ const run = function(config, hierarchyConfig, nativeEventConfig) {
// Trigger the event
const extractedEvents = ResponderEventPlugin.extractEvents(
nativeEventConfig.topLevelType,
RESPONDER_EVENT_SYSTEM,
Copy link
Contributor

@trueadm trueadm Sep 23, 2019

Choose a reason for hiding this comment

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

Why are we passing RESPONDER_EVENT_SYSTEM? That flag was meant for the new event system. We should be using the PLUGIN_EVENT_SYSTEM surely?

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

Looks good to me for the first stage of integration. I left a few nits.

These tests were written with replaying in mind and now we can properly
enable them.
These events only replay their last target if the target is not yet
hydrated. That way we don't have to wait for a previously hovered
boundary before invoking the current target.
That way we can check if this is a replay and therefore needs a special
case. One such special case is "mouseover" where we check the
relatedTarget.
To minimize breakages in a minor, I only do this for the new root APIs
since replaying only matters there anyway. Only if hydrating.

For Flare, I have to attach all active listeners since the current
system has one DOM listener for each. In a follow up I plan on optimizing
that by only attaching one if there's at least one active listener
which would allow us to start with only passive and then upgrade.
We need to check if the "relatedTarget" is mounted due to how the old
event system dispatches from the "out" event.
This is a follow up to facebook#16673 which didn't have a test because it wasn't
observable yet. This shows that it had a bug.
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.

None yet

6 participants