Sync hydrate discrete events in capture phase and dont replay discrete events

[salazarm]

Summary

This PR makes it so that discrete events are not replayed because there is a lot of ambiguity regarding the priority of the events and trying to enforce the order of the discrete events leads to being suboptimal with CPU. Instead we attempt to synchronously hydrate discrete events in the capture phase of the event (on the React root) in time for their capture and bubble listeners to fire naturally.

Continuous events are unaffected and will continue being sync hydrated in bubble phase if possible and replayed.

How did you test this change?

Jest.

I left the code ungated to make it easier to review but after review I will fork the codepaths and tests and put them behind a flag so that we can test the changes.

[sizebot]

Comparing: a4bc8ae...c10215e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	130.05 kB	130.05 kB	=	41.34 kB	41.34 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	132.88 kB	132.88 kB	=	42.31 kB	42.30 kB
facebook-www/ReactDOM-prod.classic.js	+0.27%	412.54 kB	413.66 kB	+0.22%	76.28 kB	76.44 kB
facebook-www/ReactDOM-prod.modern.js	+0.28%	401.13 kB	402.24 kB	+0.21%	74.56 kB	74.71 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.27%	412.54 kB	413.66 kB	+0.22%	76.28 kB	76.45 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactIs-dev.modern.js	+1.55%	9.99 kB	10.14 kB	+1.54%	2.66 kB	2.70 kB
facebook-www/ReactIs-dev.classic.js	+1.55%	9.99 kB	10.14 kB	+1.54%	2.66 kB	2.70 kB
facebook-www/JSXDEVRuntime-dev.modern.js	+0.34%	45.57 kB	45.72 kB	+0.33%	12.88 kB	12.92 kB
facebook-www/JSXDEVRuntime-dev.classic.js	+0.34%	45.57 kB	45.72 kB	+0.33%	12.88 kB	12.92 kB
facebook-www/ReactTestUtils-dev.modern.js	+0.30%	51.35 kB	51.50 kB	+0.24%	14.40 kB	14.43 kB
facebook-www/ReactTestUtils-dev.classic.js	+0.30%	51.35 kB	51.50 kB	+0.24%	14.39 kB	14.43 kB
facebook-www/ReactDOM-prod.modern.js	+0.28%	401.13 kB	402.24 kB	+0.21%	74.56 kB	74.71 kB
facebook-www/ReactDOMForked-prod.modern.js	+0.28%	401.13 kB	402.24 kB	+0.21%	74.56 kB	74.72 kB
facebook-www/ReactDOM-prod.classic.js	+0.27%	412.54 kB	413.66 kB	+0.22%	76.28 kB	76.44 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.27%	412.54 kB	413.66 kB	+0.22%	76.28 kB	76.45 kB
facebook-www/ReactDOM-profiling.modern.js	+0.26%	428.75 kB	429.87 kB	+0.21%	79.25 kB	79.42 kB
facebook-www/ReactDOMForked-profiling.modern.js	+0.26%	428.75 kB	429.87 kB	+0.21%	79.25 kB	79.42 kB
facebook-www/ReactDOM-profiling.classic.js	+0.25%	440.22 kB	441.34 kB	+0.19%	80.93 kB	81.09 kB
facebook-www/ReactDOMForked-profiling.classic.js	+0.25%	440.22 kB	441.34 kB	+0.19%	80.94 kB	81.09 kB

Generated by 🚫 dangerJS against c10215e

[salazarm]

Ping @gaearon @sebastianopeluso . Basically just deleted the discrete event replaying and moved the synchronous hydration block from queueDiscreteEvent into the event listener.

[rickhanlonii]

This looks great! Left some comments, and also think we should consider running this change in an experiment. Will followup offline 👍

[salazarm]

Gated the changes so we can test it. Gated some of the tests too but for some I checked the flag within the test since I didn't want to invalidate the whole test when the flag was on

[gaearon]

let's try it, but please fix up reduced coverage and skip unrelated changes before merging

[gaearon]

What happens if we're not ready to hydrate? Do we prevent the event propagation in the capture phase? I think we planned to do this but I'm not sure if that's still the plan, and whether this is how it works in this PR.

[salazarm]

What happens if we're not ready to hydrate? Do we prevent the event propagation in the capture phase? I think we planned to do this but I'm not sure if that's still the plan, and whether this is how it works in this PR.

Yeah thats a mistake on my part I should have added that in the PR too but it felt like we were less confident about that part of the change. Here are the notes from when we spoke about it:

• Use capture phase on root to attempt to sync hydrate, if it fails calls stopPropagation. Ideally, it would allow it to bubble, but we’ll try without it first. You can’t re-dispatch because it would re-fire the capture phase. The downside is that things above will not be bubble, so either they’ll miss events, or they’ll only see the full phase before react starts hydrating.
  * Another option is to only disable React events but we want to get away from React events being special
  * This also has the preventDefault issue, where react events contain preventDefaults, that would prevent the native events from firing if we were actually hydrated

So It sounds like we wanted to test:

1. Calling stopPropagation if hydration fails
2. Allowing it to bubble if hydration fails

This current PR does (2) but I can put up a follow up PR to test (1) first since thats what we agreed to

[gaearon]

I'm a bit confused by what you mean by "to test". I'm speaking more about the behavior itself, not only the tests. What is the current behavior, and what is the behavior that was agreed to? I wasn't actively tracking the discussion so maybe it would be good if you could confirm with Seb what the latest decision was. Either it should stop propagation during unhydrated capture, or it should not.

[sebmarkbage]

I'm a bit confused by what you mean by "to test".

I think he meant "to test" as in "to try". I think that's the decision. To see what happens in practice.

I think we'll want to try (1) instead though.

Allowing it to continue means that it doesn't just bubble up the parent but also down the children. I think we'll want to add a test that the parent listener doesn't ever fire in this case:

let triggeredParent = false;
...
<div onClick={() => triggeredParent = true}>
  <Suspense fallback={null}>
    <div onClick={e => e.stopPropagation()} ref={ref} />
  </Suspense>
</div>
...
ref.current.dispatchEvent(...);
...
expect(triggeredParent).toBe(false);

These days, so many apps use React all the way to the <body> - the impact of what happens outside the React root is much more scoped. It seems best to at least preserve consistent the semantics within the root which would be most adhoc stuff.

[gaearon]

I think he meant "to test" as in "to try".

Ahh ok I get it now. 😛

So the conclusion is let's do a follow-up for (1) with the test you described before enabling the flag.

[salazarm]

Interestingly I wrote the test but it already passes without any other changes🤔

[salazarm]

#22502

[sebmarkbage]

Let's try rolling this out quickly and then move on to cleaning up the code. Because the code structure now is way over-engineered for what it's actually doing and keeping this structure will just lead to confusion and dogma about keeping the structure.

I'd like to see what this looks like with all the unnecessary code removed and the attempt refactored.

Also, the goal is to eventually get rid of the synthetic event system. This dispatch function only exists for that purposes.

To explain this layering it would be good if that all the sync hydration stuff was the first thing that happened in the listener without early bailout - or even it's own listener. That way we know that the system behaves the same with React events as with other event listeners.

For example the whole attemptToDispatchEvent and blockedOn structure probably doesn't make sense anymore.