Skip to content

Commit

Permalink
Re-land "Fix: flushSync changes priority inside effect (facebook#21122)"
Browse files Browse the repository at this point in the history
This re-lands commit 0e3c7e1.
  • Loading branch information
acdlite committed May 3, 2021
1 parent df420bc commit 098600c
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 22 deletions.
22 changes: 11 additions & 11 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1142,16 +1142,6 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {

export function flushSync<A, R>(fn: A => R, a: A): R {
const prevExecutionContext = executionContext;
if ((prevExecutionContext & (RenderContext | CommitContext)) !== NoContext) {
if (__DEV__) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
return fn(a);
}
executionContext |= BatchedContext;

const previousPriority = getCurrentUpdatePriority();
Expand All @@ -1168,7 +1158,17 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
// Flush the immediate callbacks that were scheduled during this batch.
// Note that this will happen even if batchedUpdates is higher up
// the stack.
flushSyncCallbacks();
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
flushSyncCallbacks();
} else {
if (__DEV__) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
}
}
}

Expand Down
22 changes: 11 additions & 11 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1142,16 +1142,6 @@ export function unbatchedUpdates<A, R>(fn: (a: A) => R, a: A): R {

export function flushSync<A, R>(fn: A => R, a: A): R {
const prevExecutionContext = executionContext;
if ((prevExecutionContext & (RenderContext | CommitContext)) !== NoContext) {
if (__DEV__) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
return fn(a);
}
executionContext |= BatchedContext;

const previousPriority = getCurrentUpdatePriority();
Expand All @@ -1168,7 +1158,17 @@ export function flushSync<A, R>(fn: A => R, a: A): R {
// Flush the immediate callbacks that were scheduled during this batch.
// Note that this will happen even if batchedUpdates is higher up
// the stack.
flushSyncCallbacks();
if ((executionContext & (RenderContext | CommitContext)) === NoContext) {
flushSyncCallbacks();
} else {
if (__DEV__) {
console.error(
'flushSync was called from inside a lifecycle method. React cannot ' +
'flush when React is already rendering. Consider moving this call to ' +
'a scheduler task or micro task.',
);
}
}
}
}

Expand Down
64 changes: 64 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
let React;
let ReactNoop;
let Scheduler;
let useState;
let useEffect;

describe('ReactFlushSync', () => {
beforeEach(() => {
jest.resetModules();

React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
useState = React.useState;
useEffect = React.useEffect;
});

function Text({text}) {
Scheduler.unstable_yieldValue(text);
return text;
}

// @gate experimental || !enableSyncDefaultUpdates
test('changes priority of updates in useEffect', async () => {
function App() {
const [syncState, setSyncState] = useState(0);
const [state, setState] = useState(0);
useEffect(() => {
if (syncState !== 1) {
setState(1);
ReactNoop.flushSync(() => setSyncState(1));
}
}, [syncState, state]);
return <Text text={`${syncState}, ${state}`} />;
}

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

// The passive effect will schedule a sync update and a normal update.
// They should commit in two separate batches. First the sync one.
expect(() => {
expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
}).toErrorDev('flushSync was called from inside a lifecycle method');

// The remaining update is not sync
ReactNoop.flushSync();
expect(Scheduler).toHaveYielded([]);

// Now flush it.
expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
});
expect(root).toMatchRenderedOutput('1, 1');
});
});

0 comments on commit 098600c

Please sign in to comment.