Skip to content

Commit

Permalink
Clean up enableSyncDefaultUpdates flag a bit (facebook#26858)
Browse files Browse the repository at this point in the history
## 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`.
  • Loading branch information
rickhanlonii authored and AndyPengc12 committed Apr 15, 2024
1 parent c640565 commit 76158e3
Show file tree
Hide file tree
Showing 35 changed files with 316 additions and 882 deletions.
2 changes: 1 addition & 1 deletion packages/react-art/src/__tests__/ReactART-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ describe('ReactART', () => {
expect(onClick2).toBeCalled();
});

// @gate !enableSyncDefaultUpdates
// @gate forceConcurrentByDefaultForTesting
it('can concurrently render with a "primary" renderer while sharing context', async () => {
const CurrentRendererContext = React.createContext(null);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,10 +312,10 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
expect(container.textContent).toEqual('not hovered');

await waitFor(['hovered']);
if (gate(flags => flags.enableSyncDefaultUpdates)) {
expect(container.textContent).toEqual('hovered');
} else {
if (gate(flags => flags.forceConcurrentByDefaultForTesting)) {
expect(container.textContent).toEqual('not hovered');
} else {
expect(container.textContent).toEqual('hovered');
}
});
expect(container.textContent).toEqual('hovered');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2036,13 +2036,13 @@ describe('ReactDOMServerPartialHydration', () => {
suspend = true;

await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
await waitFor(['Before', 'After']);
} else {
if (gate(flags => flags.forceConcurrentByDefaultForTesting)) {
await waitFor(['Before']);
// This took a long time to render.
Scheduler.unstable_advanceTime(1000);
await waitFor(['After']);
} else {
await waitFor(['Before', 'After']);
}

// This will cause us to skip the second row completely.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1984,13 +1984,9 @@ describe('DOMPluginEventSystem', () => {
log.length = 0;

// Increase counter
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<Test counter={1} />);
});
} else {
React.startTransition(() => {
root.render(<Test counter={1} />);
}
});
// Yield before committing
await waitFor(['Test']);

Expand Down
11 changes: 7 additions & 4 deletions packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import {
enableProfilerTimer,
enableScopeAPI,
enableLegacyHidden,
enableSyncDefaultUpdates,
forceConcurrentByDefaultForTesting,
allowConcurrentByDefault,
enableTransitionTracing,
enableDebugTracing,
Expand Down Expand Up @@ -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 ||
forceConcurrentByDefaultForTesting
) {
mode |= ConcurrentUpdatesByDefaultMode;
} else if (
// Only for internal experiments.
(allowConcurrentByDefault && concurrentUpdatesByDefaultOverride)
allowConcurrentByDefault &&
concurrentUpdatesByDefaultOverride
) {
mode |= ConcurrentUpdatesByDefaultMode;
}
Expand Down
160 changes: 62 additions & 98 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,22 @@ describe('ReactExpiration', () => {
}

it('increases priority of updates as time progresses', async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
if (gate(flags => flags.forceConcurrentByDefaultForTesting)) {
ReactNoop.render(<span prop="done" />);
expect(ReactNoop).toMatchRenderedOutput(null);

// Nothing has expired yet because time hasn't advanced.
flushNextRenderIfExpired();
expect(ReactNoop).toMatchRenderedOutput(null);
// Advance time a bit, but not enough to expire the low pri update.
ReactNoop.expire(4500);
flushNextRenderIfExpired();
expect(ReactNoop).toMatchRenderedOutput(null);
// Advance by another second. Now the update should expire and flush.
ReactNoop.expire(500);
flushNextRenderIfExpired();
expect(ReactNoop).toMatchRenderedOutput(<span prop="done" />);
} else {
ReactNoop.render(<Text text="Step 1" />);
React.startTransition(() => {
ReactNoop.render(<Text text="Step 2" />);
Expand All @@ -147,21 +162,6 @@ describe('ReactExpiration', () => {
ReactNoop.expire(500);
await unstable_waitForExpired(['Step 2']);
expect(ReactNoop).toMatchRenderedOutput('Step 2');
} else {
ReactNoop.render(<span prop="done" />);
expect(ReactNoop).toMatchRenderedOutput(null);

// Nothing has expired yet because time hasn't advanced.
flushNextRenderIfExpired();
expect(ReactNoop).toMatchRenderedOutput(null);
// Advance time a bit, but not enough to expire the low pri update.
ReactNoop.expire(4500);
flushNextRenderIfExpired();
expect(ReactNoop).toMatchRenderedOutput(null);
// Advance by another second. Now the update should expire and flush.
ReactNoop.expire(500);
flushNextRenderIfExpired();
expect(ReactNoop).toMatchRenderedOutput(<span prop="done" />);
}
});

Expand All @@ -187,13 +187,9 @@ describe('ReactExpiration', () => {

// First, show what happens for updates in two separate events.
// Schedule an update.
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render(<TextClass text="A" />);
});
} else {
React.startTransition(() => {
ReactNoop.render(<TextClass text="A" />);
}
});
// Advance the timer.
Scheduler.unstable_advanceTime(2000);
// Partially flush the first update, then interrupt it.
Expand Down Expand Up @@ -248,13 +244,10 @@ describe('ReactExpiration', () => {

// First, show what happens for updates in two separate events.
// Schedule an update.
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render(<TextClass text="A" />);
});
} else {
React.startTransition(() => {
ReactNoop.render(<TextClass text="A" />);
}
});

// Advance the timer.
Scheduler.unstable_advanceTime(2000);
// Partially flush the first update, then interrupt it.
Expand Down Expand Up @@ -320,13 +313,10 @@ describe('ReactExpiration', () => {
}

// Initial mount
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render(<App />);
});
} else {
React.startTransition(() => {
ReactNoop.render(<App />);
}
});

await waitForAll([
'initial [A] [render]',
'initial [B] [render]',
Expand All @@ -339,13 +329,10 @@ describe('ReactExpiration', () => {
]);

// Partial update
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
subscribers.forEach(s => s.setState({text: '1'}));
});
} else {
React.startTransition(() => {
subscribers.forEach(s => s.setState({text: '1'}));
}
});

await waitFor(['1 [A] [render]', '1 [B] [render]']);

// Before the update can finish, update again. Even though no time has
Expand All @@ -371,13 +358,9 @@ describe('ReactExpiration', () => {
);
}

if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App />);
});
} else {
React.startTransition(() => {
root.render(<App />);
}
});

await waitFor(['A']);
await waitFor(['B']);
Expand All @@ -404,13 +387,9 @@ describe('ReactExpiration', () => {
</>
);
}
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App />);
});
} else {
React.startTransition(() => {
root.render(<App />);
}
});

await waitFor(['A']);
await waitFor(['B']);
Expand All @@ -429,7 +408,26 @@ describe('ReactExpiration', () => {
jest.resetModules();
Scheduler = require('scheduler');

if (gate(flags => flags.enableSyncDefaultUpdates)) {
if (gate(flags => flags.forceConcurrentByDefaultForTesting)) {
// Before importing the renderer, advance the current time by a number
// larger than the maximum allowed for bitwise operations.
const maxSigned31BitInt = 1073741823;
Scheduler.unstable_advanceTime(maxSigned31BitInt * 100);
// Now import the renderer. On module initialization, it will read the
// current time.
ReactNoop = require('react-noop-renderer');
ReactNoop.render('Hi');

// The update should not have expired yet.
flushNextRenderIfExpired();
await waitFor([]);
expect(ReactNoop).toMatchRenderedOutput(null);
// Advance the time some more to expire the update.
Scheduler.unstable_advanceTime(10000);
flushNextRenderIfExpired();
await waitFor([]);
expect(ReactNoop).toMatchRenderedOutput('Hi');
} else {
const InternalTestUtils = require('internal-test-utils');
waitFor = InternalTestUtils.waitFor;
assertLog = InternalTestUtils.assertLog;
Expand All @@ -446,14 +444,10 @@ describe('ReactExpiration', () => {
React = require('react');

ReactNoop.render(<Text text="Step 1" />);
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render(<Text text="Step 2" />);
});
await waitFor(['Step 1']);
} else {
ReactNoop.render('Hi');
}
React.startTransition(() => {
ReactNoop.render(<Text text="Step 2" />);
});
await waitFor(['Step 1']);

// The update should not have expired yet.
await unstable_waitForExpired([]);
Expand All @@ -464,25 +458,6 @@ describe('ReactExpiration', () => {
Scheduler.unstable_advanceTime(10000);
await unstable_waitForExpired(['Step 2']);
expect(ReactNoop).toMatchRenderedOutput('Step 2');
} else {
// Before importing the renderer, advance the current time by a number
// larger than the maximum allowed for bitwise operations.
const maxSigned31BitInt = 1073741823;
Scheduler.unstable_advanceTime(maxSigned31BitInt * 100);
// Now import the renderer. On module initialization, it will read the
// current time.
ReactNoop = require('react-noop-renderer');
ReactNoop.render('Hi');

// The update should not have expired yet.
flushNextRenderIfExpired();
await waitFor([]);
expect(ReactNoop).toMatchRenderedOutput(null);
// Advance the time some more to expire the update.
Scheduler.unstable_advanceTime(10000);
flushNextRenderIfExpired();
await waitFor([]);
expect(ReactNoop).toMatchRenderedOutput('Hi');
}
});

Expand All @@ -494,13 +469,10 @@ describe('ReactExpiration', () => {
// Before scheduling an update, advance the current time.
Scheduler.unstable_advanceTime(10000);

if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render('Hi');
});
} else {
React.startTransition(() => {
ReactNoop.render('Hi');
}
});

await unstable_waitForExpired([]);
expect(ReactNoop).toMatchRenderedOutput(null);

Expand Down Expand Up @@ -541,13 +513,9 @@ describe('ReactExpiration', () => {

// First demonstrate what happens when there's no starvation
await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
updateNormalPri();
});
} else {
React.startTransition(() => {
updateNormalPri();
}
});
await waitFor(['Sync pri: 0']);
updateSyncPri();
assertLog(['Sync pri: 1', 'Normal pri: 0']);
Expand All @@ -565,13 +533,9 @@ describe('ReactExpiration', () => {

// Do the same thing, but starve the first update
await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
updateNormalPri();
});
} else {
React.startTransition(() => {
updateNormalPri();
}
});
await waitFor(['Sync pri: 1']);

// This time, a lot of time has elapsed since the normal pri update
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,9 @@ describe('ReactFlushSync', () => {

const root = ReactNoop.createRoot();
await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App />);
});
} else {
React.startTransition(() => {
root.render(<App />);
}
});
// This will yield right before the passive effect fires
await waitForPaint(['0, 0']);

Expand Down
Loading

0 comments on commit 76158e3

Please sign in to comment.