Convert ReactDOMEventListener to createRoot #28050

rickhanlonii · 2024-01-22T16:28:00Z

No description provided.

react-sizebot · 2024-01-22T16:34:31Z

Comparing: 64d0c94...9cd87d6

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	=	176.24 kB	176.24 kB	=	54.88 kB	54.88 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.34 kB	178.34 kB	=	55.53 kB	55.53 kB
facebook-www/ReactDOM-prod.classic.js	=	571.94 kB	571.94 kB	=	100.56 kB	100.56 kB
facebook-www/ReactDOM-prod.modern.js	=	555.70 kB	555.70 kB	=	97.62 kB	97.62 kB
test_utils/ReactAllWarnings.js	Deleted	67.55 kB	0.00 kB	Deleted	16.54 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	67.55 kB	0.00 kB	Deleted	16.54 kB	0.00 kB

Generated by 🚫 dangerJS against 9cd87d6

rickhanlonii · 2024-01-22T16:31:33Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

-        expect(mouseOut.mock.calls[2][0]).toEqual(grandParentNode);
+        expect(mouseOut).toHaveBeenNthCalledWith(1, childNode);
+        expect(mouseOut).toHaveBeenNthCalledWith(2, parentNode);
+        expect(mouseOut).toHaveBeenNthCalledWith(3, grandParentNode);


I switched these mock assertions to toHaveBeenNthCalledWith. I also removed .toBeCalled since that's covered by toHaveBeenCalledTimes.

rickhanlonii · 2024-01-22T16:34:24Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

        expect(container.firstChild.textContent).toBe('clicked!');
      } finally {
        document.body.removeChild(container);
      }
    });

-    it('should batch between handlers from different roots', () => {
+    it('should batch between handlers from different roots', async () => {


This test has major changes. In the legacy root, batching across roots was fucky - it flushed the batched updates from the events in the other root. In createRoot, updates are batched in events across roots. This is similar to the behavior the test had before the modern event system, as you can see in the PR here: https://github.com/facebook/react/pull/18195/files#diff-1c4be0ca4ba2113d6a748ad7059f72181a50021f5a447ec797ca2f6d3d607a79R214

I don't know exactly what changes we made in createRoot that changed this, but I know we improved the batching.

@sebmarkbage could you take a quick look at this test and confirm the new behavior is expected?

I think this is not quite faithfully reproducing what a browser would do.

What this is testing is that we have two nested containers and that each container has event delegation listeners on them.

The event will bubble through both and so trigger two event delegation events into React. Ideally they'd be batched but we need some time to flush them.

For some events we can flush in a macro task such as at rAF. However, for discrete events this is too late. There is no event that is guaranteed to fire later than the current container event listener and before the next discrete event. Ideally on the window but this is not guaranteed to fire at all if propagation is stopped.

So for discrete events we use microtask. Unfortunately, IIRC, microtasks fire between every bubbled event. When you manually dispatchEvent this doesn't happen because it's already in a macrotask but the browser's dispatch schedules separate macrotasks for each bubbled event. Therefore for a discrete flush it should flush between the two different events.

You're not giving it an opportunity to flush microtasks between the bubbling which the browser does. So to simulate that you might have to manually fire the bubbling and yield with a couple of awaits between each firing. To do this correctly you might have to patch the target on the event object.

Now, for this particular test I don't know exactly the semantics because it's using a mouseout event which is not a discrete event so even with the right testing it might "work" by batching.

Therefore I think we should make a duplicate of this same test with a discrete event too like click because it should definitely not work there.

Ok, I reverted the changes to the test in this PR, and followed up with a fix using the strategy you outlined here: #28079

Also added a separate test so we cover both discrete and continuous, which shows the different expected behavior.

rickhanlonii · 2024-01-22T16:34:58Z

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

+      );
+    });
+
+    await act(() => {


I'm wrapping all these dispatchEvents in act because that's the right thing to do, even though in these cases it works without it.

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js

eps1lon

Looks good but the "should batch between handlers from different roots" test could use another set of eyes.

@sebmarkbage

## Overview For events, the browser will yield to microtasks between calling event handers, allowing time to flush work inbetween. For example, in the browser, this code will log the flushes between events: ```js <body onclick="console.log('body'); Promise.resolve().then(() => console.log('flush body'));"> <div onclick="console.log('div'); Promise.resolve().then(() => console.log('flush div'));"> hi </div> </body> // Logs div flush div body flush body ``` [Sandbox](https://codesandbox.io/s/eloquent-noether-mw2cjg?file=/index.html) The problem is, `dispatchEvent` (either in the browser, or JSDOM) does not yield to microtasks. Which means, this code will log the flushes after the events: ```js const target = document.getElementsByTagName("div")[0]; const nativeEvent = document.createEvent("Event"); nativeEvent.initEvent("click", true, true); target.dispatchEvent(nativeEvent); // Logs div body flush div flush body ``` ## The problem This mostly isn't a problem because React attaches event handler at the root, and calls the event handlers on components via the synthetic event system. We handle flushing between calling event handlers as needed. However, if you're mixing capture and bubbling events, or using multiple roots, then the problem of not flushing microtasks between events can come into play. This was found when converting a test to `createRoot` in #28050 (comment), and that test is an example of where this is an issue with nested roots. Here's a sandox for [discrete](https://codesandbox.io/p/sandbox/red-http-2wg8k5) and [continuous](https://codesandbox.io/p/sandbox/gracious-voice-6r7tsc?file=%2Fsrc%2Findex.js%3A25%2C28) events, showing how the test should behave. The existing test, when switched to `createRoot` matches the browser behavior for continuous events, but not discrete. Continuous events should be batched, and discrete should flush individually. ## The fix This PR implements the fix suggested by @sebmarkbage, to manually traverse the path up from the element and dispatch events, yielding between each call.

@sebmarkbage

## Overview For events, the browser will yield to microtasks between calling event handers, allowing time to flush work inbetween. For example, in the browser, this code will log the flushes between events: ```js <body onclick="console.log('body'); Promise.resolve().then(() => console.log('flush body'));"> <div onclick="console.log('div'); Promise.resolve().then(() => console.log('flush div'));"> hi </div> </body> // Logs div flush div body flush body ``` [Sandbox](https://codesandbox.io/s/eloquent-noether-mw2cjg?file=/index.html) The problem is, `dispatchEvent` (either in the browser, or JSDOM) does not yield to microtasks. Which means, this code will log the flushes after the events: ```js const target = document.getElementsByTagName("div")[0]; const nativeEvent = document.createEvent("Event"); nativeEvent.initEvent("click", true, true); target.dispatchEvent(nativeEvent); // Logs div body flush div flush body ``` ## The problem This mostly isn't a problem because React attaches event handler at the root, and calls the event handlers on components via the synthetic event system. We handle flushing between calling event handlers as needed. However, if you're mixing capture and bubbling events, or using multiple roots, then the problem of not flushing microtasks between events can come into play. This was found when converting a test to `createRoot` in facebook#28050 (comment), and that test is an example of where this is an issue with nested roots. Here's a sandox for [discrete](https://codesandbox.io/p/sandbox/red-http-2wg8k5) and [continuous](https://codesandbox.io/p/sandbox/gracious-voice-6r7tsc?file=%2Fsrc%2Findex.js%3A25%2C28) events, showing how the test should behave. The existing test, when switched to `createRoot` matches the browser behavior for continuous events, but not discrete. Continuous events should be batched, and discrete should flush individually. ## The fix This PR implements the fix suggested by @sebmarkbage, to manually traverse the path up from the element and dispatch events, yielding between each call.

DiffTrain build for commit 2efb142.

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 22, 2024

Convert ReactDOMEventListener to createRoot

4235b3e

rickhanlonii force-pushed the rh/test-event-listener branch from dd103a6 to 4235b3e Compare January 22, 2024 16:30

rickhanlonii commented Jan 22, 2024

View reviewed changes

eps1lon reviewed Jan 23, 2024

View reviewed changes

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js Outdated Show resolved Hide resolved

packages/react-dom/src/__tests__/ReactDOMEventListener-test.js Outdated Show resolved Hide resolved

Fix innerRef and ReactDOMX

ed36661

eps1lon approved these changes Jan 24, 2024

View reviewed changes

rickhanlonii added 2 commits January 24, 2024 21:32

Revert batching test back, will follow up

3cfeb68

Fix lint

9cd87d6

rickhanlonii merged commit 2efb142 into facebook:main Jan 25, 2024
36 checks passed

rickhanlonii deleted the rh/test-event-listener branch January 25, 2024 04:17

rickhanlonii mentioned this pull request Jan 25, 2024

Add simulateEventDispatch to test ReactDOMEventListener #28079

Merged

gnoff mentioned this pull request Feb 1, 2024

Update React from 60a927d04 to 2bc7d336a vercel/next.js#61522

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024

Convert ReactDOMEventListener to createRoot (facebook#28050)

8fe4d26

bigfootjon pushed a commit that referenced this pull request Apr 18, 2024

Convert ReactDOMEventListener to createRoot (#28050)

95c2763

DiffTrain build for commit 2efb142.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert ReactDOMEventListener to createRoot #28050

Convert ReactDOMEventListener to createRoot #28050

rickhanlonii commented Jan 22, 2024

react-sizebot commented Jan 22, 2024 •

edited

Loading

rickhanlonii Jan 22, 2024

rickhanlonii Jan 22, 2024

sebmarkbage Jan 24, 2024

rickhanlonii Jan 25, 2024

rickhanlonii Jan 22, 2024

eps1lon left a comment

Convert ReactDOMEventListener to createRoot #28050

Convert ReactDOMEventListener to createRoot #28050

Conversation

rickhanlonii commented Jan 22, 2024

react-sizebot commented Jan 22, 2024 • edited Loading

Critical size changes

Significant size changes

rickhanlonii Jan 22, 2024

Choose a reason for hiding this comment

rickhanlonii Jan 22, 2024

Choose a reason for hiding this comment

sebmarkbage Jan 24, 2024

Choose a reason for hiding this comment

rickhanlonii Jan 25, 2024

Choose a reason for hiding this comment

rickhanlonii Jan 22, 2024

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

react-sizebot commented Jan 22, 2024 •

edited

Loading