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

[Test] Update flushSync tests to use react-dom #28490

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Mar 5, 2024

Updates the flushSync tests to use react-dom instead of ReactNoop. flushSync is primarily a react-dom API and asserting the implementation of ReactNoop is not really ideal especially since we are going to be refactoring flushSync soon to not be implemented in the reconciler internals.

ReactNoop still have a flushSync api and it can still be used in other tests that are primarily about testing other functionlity and use ReactNoop as the renderer.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 5, 2024
@gnoff gnoff requested a review from acdlite March 5, 2024 03:19
@react-sizebot
Copy link

react-sizebot commented Mar 5, 2024

Comparing: 113ab9a...012445d

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 = 177.19 kB 177.19 kB = 55.23 kB 55.23 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 177.72 kB 177.72 kB = 55.55 kB 55.55 kB
facebook-www/ReactDOM-prod.classic.js = 594.67 kB 594.67 kB = 105.04 kB 105.04 kB
facebook-www/ReactDOM-prod.modern.js = 577.93 kB 577.93 kB = 102.10 kB 102.10 kB
test_utils/ReactAllWarnings.js Deleted 66.60 kB 0.00 kB Deleted 16.28 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 66.60 kB 0.00 kB Deleted 16.28 kB 0.00 kB

Generated by 🚫 dangerJS against 012445d

Updates the FlushSync tests to use react-dom instead of ReactNoop. flushSync is primarily a react-dom API and asserting the implementation of ReactNoop is not really ideal especially since we are going to be refactoring flushSync soon to not be implemented in the reconciler internals.

ReactNoop still have a flushSync api and it can still be used in other tests that are primarily about testing other functionlity and use ReactNoop as the renderer.
@@ -72,7 +117,7 @@ describe('ReactFlushSync', () => {
await waitForPaint(['1, 1']);
}
});
expect(root).toMatchRenderedOutput('1, 1');
expect(getVisibleChildren(container)).toEqual('1, 1');
Copy link
Member

@rickhanlonii rickhanlonii Mar 5, 2024

Choose a reason for hiding this comment

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

What do you think about creating a matcher for this? Seems common and we're starting to copy this helper over to multiple places. It would be nice to standardize the ReactDOM tests asserting container content on one patter because it's all over the place right now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it's generally very useful however there are some special cases in ReactDOMFloat tests where we do even more special casing because we need to assert certain things like scripts which the basic version of this ignores. Maybe instead of a matcher we just have a test util for this function so it's a little clearer what's happening when you choose to specialize the implementation. Or maybe the float one can be made the default one

});
});

test('do not flush passive effects synchronously after render in legacy mode', async () => {
// @gate !disableLegacyMode
Copy link
Member

Choose a reason for hiding this comment

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

woah SICK YES

@gnoff gnoff merged commit 0ae2b13 into facebook:main Mar 5, 2024
38 checks passed
@gnoff gnoff deleted the convert-flush-sync-test branch March 5, 2024 05:40
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Updates the `flushSync` tests to use react-dom instead of ReactNoop.
flushSync is primarily a react-dom API and asserting the implementation
of ReactNoop is not really ideal especially since we are going to be
refactoring flushSync soon to not be implemented in the reconciler
internals.

ReactNoop still have a flushSync api and it can still be used in other
tests that are primarily about testing other functionlity and use
ReactNoop as the renderer.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Updates the `flushSync` tests to use react-dom instead of ReactNoop.
flushSync is primarily a react-dom API and asserting the implementation
of ReactNoop is not really ideal especially since we are going to be
refactoring flushSync soon to not be implemented in the reconciler
internals.

ReactNoop still have a flushSync api and it can still be used in other
tests that are primarily about testing other functionlity and use
ReactNoop as the renderer.

DiffTrain build for commit 0ae2b13.
eps1lon added a commit to vercel/next.js that referenced this pull request Apr 19, 2024
### React upstream changes

- facebook/react#28643
- facebook/react#28628
- facebook/react#28361
- facebook/react#28513
- facebook/react#28299
- facebook/react#28617
- facebook/react#28618
- facebook/react#28621
- facebook/react#28614
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants