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

Revert Rerender Error Check #17519

Merged
merged 2 commits into from
Dec 4, 2019
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 45 additions & 41 deletions packages/react-reconciler/src/ReactFiberHooks.js
Expand Up @@ -198,7 +198,8 @@ let renderPhaseUpdates: Map<
UpdateQueue<any, any>,
Update<any, any>,
> | null = null;

// Counter to prevent infinite loops.
let numberOfReRenders: number = 0;
const RE_RENDER_LIMIT = 25;

// In DEV, this is the name of the currently executing primitive hook
Expand Down Expand Up @@ -397,6 +398,7 @@ export function renderWithHooks(

// didScheduleRenderPhaseUpdate = false;
// renderPhaseUpdates = null;
// numberOfReRenders = 0;

// TODO Warn if no hooks are used at all during mount, then some are used during update.
// Currently we will identify the update render as a mount because memoizedState === null.
Expand Down Expand Up @@ -428,17 +430,8 @@ export function renderWithHooks(
let children = Component(props, refOrContext);

if (didScheduleRenderPhaseUpdate) {
// Counter to prevent infinite loops.
let numberOfReRenders: number = 0;
do {
didScheduleRenderPhaseUpdate = false;

invariant(
numberOfReRenders < RE_RENDER_LIMIT,
'Too many re-renders. React limits the number of renders to prevent ' +
'an infinite loop.',
);

numberOfReRenders += 1;
if (__DEV__) {
// Even when hot reloading, allow dependencies to stabilize
Expand All @@ -465,6 +458,7 @@ export function renderWithHooks(
} while (didScheduleRenderPhaseUpdate);

renderPhaseUpdates = null;
numberOfReRenders = 0;
}

// We can assume the previous dispatcher is always this one, since we set it
Expand Down Expand Up @@ -495,6 +489,7 @@ export function renderWithHooks(
// These were reset above
// didScheduleRenderPhaseUpdate = false;
// renderPhaseUpdates = null;
// numberOfReRenders = 0;

invariant(
!didRenderTooFewHooks,
Expand Down Expand Up @@ -541,6 +536,7 @@ export function resetHooks(): void {

didScheduleRenderPhaseUpdate = false;
renderPhaseUpdates = null;
numberOfReRenders = 0;
}

function mountWorkInProgressHook(): Hook {
Expand Down Expand Up @@ -676,43 +672,45 @@ function updateReducer<S, I, A>(

queue.lastRenderedReducer = reducer;

if (renderPhaseUpdates !== null) {
if (numberOfReRenders > 0) {
// This is a re-render. Apply the new render phase updates to the previous
// work-in-progress hook.
const dispatch: Dispatch<A> = (queue.dispatch: any);
// Render phase updates are stored in a map of queue -> linked list
const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue);
if (firstRenderPhaseUpdate !== undefined) {
renderPhaseUpdates.delete(queue);
let newState = hook.memoizedState;
let update = firstRenderPhaseUpdate;
do {
// Process this render phase update. We don't have to check the
// priority because it will always be the same as the current
// render's.
const action = update.action;
newState = reducer(newState, action);
update = update.next;
} while (update !== null);

// Mark that the fiber performed work, but only if the new state is
// different from the current state.
if (!is(newState, hook.memoizedState)) {
markWorkInProgressReceivedUpdate();
}
if (renderPhaseUpdates !== null) {
// Render phase updates are stored in a map of queue -> linked list
const firstRenderPhaseUpdate = renderPhaseUpdates.get(queue);
if (firstRenderPhaseUpdate !== undefined) {
renderPhaseUpdates.delete(queue);
let newState = hook.memoizedState;
let update = firstRenderPhaseUpdate;
do {
// Process this render phase update. We don't have to check the
// priority because it will always be the same as the current
// render's.
const action = update.action;
newState = reducer(newState, action);
update = update.next;
} while (update !== null);

hook.memoizedState = newState;
// Don't persist the state accumulated from the render phase updates to
// the base state unless the queue is empty.
// TODO: Not sure if this is the desired semantics, but it's what we
// do for gDSFP. I can't remember why.
if (hook.baseUpdate === queue.last) {
hook.baseState = newState;
}
// Mark that the fiber performed work, but only if the new state is
// different from the current state.
if (!is(newState, hook.memoizedState)) {
markWorkInProgressReceivedUpdate();
}

hook.memoizedState = newState;
// Don't persist the state accumulated from the render phase updates to
// the base state unless the queue is empty.
// TODO: Not sure if this is the desired semantics, but it's what we
// do for gDSFP. I can't remember why.
if (hook.baseUpdate === queue.last) {
hook.baseState = newState;
}

queue.lastRenderedState = newState;
queue.lastRenderedState = newState;

return [newState, dispatch];
return [newState, dispatch];
}
}
return [hook.memoizedState, dispatch];
}
Expand Down Expand Up @@ -1205,6 +1203,12 @@ function dispatchAction<S, A>(
queue: UpdateQueue<S, A>,
action: A,
) {
invariant(
numberOfReRenders < RE_RENDER_LIMIT,
'Too many re-renders. React limits the number of renders to prevent ' +
'an infinite loop.',
);

if (__DEV__) {
warning(
typeof arguments[3] !== 'function',
Expand Down
Expand Up @@ -2493,4 +2493,35 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toHaveYielded(['Step: 5, Shadow: 5']);
expect(ReactNoop).toMatchRenderedOutput('5');
});

it('should process the rest pending updates after a render phase update', () => {
// Similar to previous test, except using a preceding render phase update
// instead of new props.
let updateA;
let updateC;
function App() {
const [a, setA] = useState(false);
const [b, setB] = useState(false);
if (a !== b) {
setB(a);
}
// Even though we called setB above,
// we should still apply the changes to C,
// during this render pass.
const [c, setC] = useState(false);
updateA = setA;
updateC = setC;
return `${a ? 'A' : 'a'}${b ? 'B' : 'b'}${c ? 'C' : 'c'}`;
}

act(() => ReactNoop.render(<App />));
expect(ReactNoop).toMatchRenderedOutput('abc');

act(() => {
updateA(true);
// This update should not get dropped.
updateC(true);
});
expect(ReactNoop).toMatchRenderedOutput('ABC');
});
});