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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 ||
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

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)) {
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.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