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.

Sign up for GitHub

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

Jump to bottom

Cleanup enableSyncDefaultUpdate flag #26236

Merged
merged 3 commits into from
Feb 27, 2023
Merged

Cleanup enableSyncDefaultUpdate flag #26236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bbc8ce
Cleanup enableSyncDefaultUpdate flag
kassens Feb 17, 2023
700d5f8
lint fix
kassens Feb 24, 2023
383d8f4
Merge branch 'main' into cleanup-enableSyncDefaultUpdates
kassens Feb 27, 2023
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
54 changes: 0 additions & 54 deletions packages/react-art/src/__tests__/ReactART-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ const ReactTestRenderer = require('react-test-renderer');

// Isolate the noop renderer
jest.resetModules();
const ReactNoop = require('react-noop-renderer');
const Scheduler = require('scheduler');

let Group;
let Shape;
Expand Down Expand Up @@ -359,58 +357,6 @@ describe('ReactART', () => {
doClick(instance);
expect(onClick2).toBeCalled();
});

// @gate !enableSyncDefaultUpdates
it('can concurrently render with a "primary" renderer while sharing context', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have context about this test, I'm curious what the output is with the flag. but since the flag is on for a while, seems it's no longer necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It yields in different order, but the code path when the @GATE doesn't match is very generic just "should fail this test when the condition doesn't match"

const CurrentRendererContext = React.createContext(null);

function Yield(props) {
Scheduler.unstable_yieldValue(props.value);
return null;
}

let ops = [];
function LogCurrentRenderer() {
return (
<CurrentRendererContext.Consumer>
{currentRenderer => {
ops.push(currentRenderer);
return null;
}}
</CurrentRendererContext.Consumer>
);
}

// Using test renderer instead of the DOM renderer here because async
// testing APIs for the DOM renderer don't exist.
ReactNoop.render(
<CurrentRendererContext.Provider value="Test">
<Yield value="A" />
<Yield value="B" />
<LogCurrentRenderer />
<Yield value="C" />
</CurrentRendererContext.Provider>,
);

expect(Scheduler).toFlushAndYieldThrough(['A']);

ReactDOM.render(
<Surface>
<LogCurrentRenderer />
<CurrentRendererContext.Provider value="ART">
<LogCurrentRenderer />
</CurrentRendererContext.Provider>
</Surface>,
container,
);

expect(ops).toEqual([null, 'ART']);

ops = [];
expect(Scheduler).toFlushAndYield(['B', 'C']);

expect(ops).toEqual(['Test']);
});
});

describe('ReactARTComponents', () => {
Expand Down
6 changes: 1 addition & 5 deletions packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
expect(container.textContent).toEqual('not hovered');

expect(Scheduler).toFlushAndYieldThrough(['hovered']);
if (gate(flags => flags.enableSyncDefaultUpdates)) {
expect(container.textContent).toEqual('hovered');
} else {
expect(container.textContent).toEqual('not hovered');
}
expect(container.textContent).toEqual('hovered');
});
expect(container.textContent).toEqual('hovered');
});
Expand Down
9 changes: 1 addition & 8 deletions packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2044,14 +2044,7 @@ describe('ReactDOMServerPartialHydration', () => {
suspend = true;

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

// This will cause us to skip the second row completely.
});
Expand Down
8 changes: 2 additions & 6 deletions packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1965,13 +1965,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
expect(Scheduler).toFlushAndYieldThrough(['Test']);

Expand Down
7 changes: 2 additions & 5 deletions packages/react-reconciler/src/ReactFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import {
enableProfilerTimer,
enableScopeAPI,
enableLegacyHidden,
enableSyncDefaultUpdates,
allowConcurrentByDefault,
enableTransitionTracing,
enableDebugTracing,
Expand Down Expand Up @@ -459,11 +458,9 @@ export function createHostRootFiber(
mode |= StrictLegacyMode | StrictEffectsMode;
}
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 ||
// Only for internal experiments.
(allowConcurrentByDefault && concurrentUpdatesByDefaultOverride)
allowConcurrentByDefault &&
concurrentUpdatesByDefaultOverride
) {
mode |= ConcurrentUpdatesByDefaultMode;
}
Expand Down
8 changes: 2 additions & 6 deletions ...ler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,9 @@ describe('ReactSuspenseList', () => {
root.render(<App show={false} />);
expect(Scheduler).toFlushAndYield([]);

if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App show={true} />);
});
} else {
React.startTransition(() => {
root.render(<App show={true} />);
}
});
expect(Scheduler).toFlushAndYield([
'Suspend! [A]',
'Suspend! [B]',
Expand Down
96 changes: 24 additions & 72 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,9 @@ describe('ReactExpiration', () => {
}

it('increases priority of updates as time progresses', () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render(<span prop="done" />);
});
} else {
React.startTransition(() => {
ReactNoop.render(<span prop="done" />);
}
});

expect(ReactNoop).toMatchRenderedOutput(null);

Expand Down Expand Up @@ -162,13 +158,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 @@ -223,13 +215,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 @@ -301,13 +289,9 @@ describe('ReactExpiration', () => {
}

// Initial mount
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
ReactNoop.render(<App />);
});
} else {
React.startTransition(() => {
ReactNoop.render(<App />);
}
});
expect(Scheduler).toFlushAndYield([
'initial [A] [render]',
'initial [B] [render]',
Expand All @@ -320,13 +304,9 @@ 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'}));
}
});
expect(Scheduler).toFlushAndYieldThrough([
'1 [A] [render]',
'1 [B] [render]',
Expand Down Expand Up @@ -358,13 +338,9 @@ describe('ReactExpiration', () => {
);
}

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

expect(Scheduler).toFlushAndYieldThrough(['A']);
expect(Scheduler).toFlushAndYieldThrough(['B']);
Expand Down Expand Up @@ -392,13 +368,9 @@ describe('ReactExpiration', () => {
</>
);
}
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App />);
});
} else {
React.startTransition(() => {
root.render(<App />);
}
});

expect(Scheduler).toFlushAndYieldThrough(['A']);
expect(Scheduler).toFlushAndYieldThrough(['B']);
Expand Down Expand Up @@ -426,13 +398,9 @@ describe('ReactExpiration', () => {
// current time.
ReactNoop = require('react-noop-renderer');

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

// The update should not have expired yet.
flushNextRenderIfExpired();
Expand All @@ -455,13 +423,9 @@ 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');
}
});
flushNextRenderIfExpired();
expect(Scheduler).toHaveYielded([]);
expect(ReactNoop).toMatchRenderedOutput(null);
Expand Down Expand Up @@ -504,13 +468,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();
}
});
expect(Scheduler).toFlushAndYieldThrough(['Sync pri: 0']);
updateSyncPri();
expect(Scheduler).toHaveYielded(['Sync pri: 1', 'Normal pri: 0']);
Expand All @@ -528,13 +488,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();
}
});
expect(Scheduler).toFlushAndYieldThrough(['Sync pri: 1']);

// This time, a lot of time has elapsed since the normal pri update
Expand Down Expand Up @@ -690,13 +646,9 @@ describe('ReactExpiration', () => {
expect(root).toMatchRenderedOutput('A0BC');

await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
React.startTransition(() => {
root.render(<App step={1} />);
});
} else {
React.startTransition(() => {
root.render(<App step={1} />);
}
});
expect(Scheduler).toFlushAndYield([
'Suspend! [A1]',
'B',
Expand Down
8 changes: 2 additions & 6 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,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
expect(Scheduler).toFlushUntilNextPaint(['0, 0']);

Expand Down
Loading