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

Clean up enableSyncDefaultUpdates flag a bit #26858

Merged
merged 5 commits into from
Jun 1, 2023

Conversation

rickhanlonii
Copy link
Member

Overview

Does a few things:

  • Renames enableSyncDefaultUpdates to forceConcurrentByDefaultForTesting
  • Changes the way it's used so it's dead-code eliminated separate from allowConcurrentByDefault
  • Deletes a bunch of the gated code

The gates that are deleted are unnecessary now. We were keeping them when we originally thought we would come back to being concurrent by default. But we've shifted and now sync-by default is the desired behavior long term, so there's no need to keep all these forked tests around.

I'll follow up to delete more of the forked behavior if possible. Ideally we wouldn't need this flag even if we're still using allowConcurrentByDefault.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 26, 2023
@@ -460,10 +460,13 @@ export function createHostRootFiber(
}
if (
// We only use this flag for our repo tests to check both behaviors.
// TODO: Flip this flag and rename it something like "forceConcurrentByDefaultForTesting"
!enableSyncDefaultUpdates ||
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why i wrote this clowny ass code this way before

@@ -320,13 +313,10 @@ describe('ReactExpiration', () => {
}

// Initial mount
if (gate(flags => flags.enableSyncDefaultUpdates)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Most of these gate checks just switch to use transitions if we're sync by default. But we can just always use transitions, since that what we'll need to do long term to do these tests anyway.

@react-sizebot
Copy link

react-sizebot commented May 26, 2023

Comparing: 0210f0b...aa86e10

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 = 164.23 kB 164.23 kB = 51.77 kB 51.77 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.59 kB 171.59 kB = 53.98 kB 53.98 kB
facebook-www/ReactDOM-prod.classic.js = 570.42 kB 570.42 kB = 100.64 kB 100.64 kB
facebook-www/ReactDOM-prod.modern.js = 554.16 kB 554.16 kB = 97.82 kB 97.82 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against aa86e10

@rickhanlonii rickhanlonii merged commit 018c58c into facebook:main Jun 1, 2023
@rickhanlonii rickhanlonii deleted the rh/sync-flag-updates branch June 1, 2023 13:24
github-actions bot pushed a commit that referenced this pull request Jun 1, 2023
## Overview

Does a few things:
- Renames `enableSyncDefaultUpdates` to
`forceConcurrentByDefaultForTesting`
- Changes the way it's used so it's dead-code eliminated separate from
`allowConcurrentByDefault`
- Deletes a bunch of the gated code

The gates that are deleted are unnecessary now. We were keeping them
when we originally thought we would come back to being concurrent by
default. But we've shifted and now sync-by default is the desired
behavior long term, so there's no need to keep all these forked tests
around.

I'll follow up to delete more of the forked behavior if possible.
Ideally we wouldn't need this flag even if we're still using
`allowConcurrentByDefault`.

DiffTrain build for [018c58c](018c58c)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
## Overview

Does a few things:
- Renames `enableSyncDefaultUpdates` to
`forceConcurrentByDefaultForTesting`
- Changes the way it's used so it's dead-code eliminated separate from
`allowConcurrentByDefault`
- Deletes a bunch of the gated code

The gates that are deleted are unnecessary now. We were keeping them
when we originally thought we would come back to being concurrent by
default. But we've shifted and now sync-by default is the desired
behavior long term, so there's no need to keep all these forked tests
around.

I'll follow up to delete more of the forked behavior if possible.
Ideally we wouldn't need this flag even if we're still using
`allowConcurrentByDefault`.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
## Overview

Does a few things:
- Renames `enableSyncDefaultUpdates` to
`forceConcurrentByDefaultForTesting`
- Changes the way it's used so it's dead-code eliminated separate from
`allowConcurrentByDefault`
- Deletes a bunch of the gated code

The gates that are deleted are unnecessary now. We were keeping them
when we originally thought we would come back to being concurrent by
default. But we've shifted and now sync-by default is the desired
behavior long term, so there's no need to keep all these forked tests
around.

I'll follow up to delete more of the forked behavior if possible.
Ideally we wouldn't need this flag even if we're still using
`allowConcurrentByDefault`.

DiffTrain build for commit 018c58c.
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