Skip to content

Commit

Permalink
Initialize update queue object on mount (#17560)
Browse files Browse the repository at this point in the history
* Refactor Update Queues to Fix Rebasing Bug

Fixes a bug related to rebasing updates. Once an update has committed,
it should never un-commit, even if interrupted by a higher priority
update. The fix includes a refactor of how update queues work.

This commit is a combination of two PRs:

- #17483 by @sebmarkbage refactors the hook update queue
- #17510 by @acdlite refactors the class and root update queue

Landing one without the other would cause state updates to sometimes be
inconsistent across components, so I've combined them into a single
commit in case they need to be reverted.

Co-authored-by: Sebastian Markbåge <sema@fb.com>
Co-authored-by: Andrew Clark <git@andrewclark.io>

* Initialize update queue object on mount

Instead of lazily initializing update queue objects on the first update,
class and host root queues are created on mount. This simplifies the
logic for appending new updates and matches what we do for hooks.
  • Loading branch information
acdlite authored Dec 11, 2019
1 parent 031a5aa commit 7bf40e1
Show file tree
Hide file tree
Showing 8 changed files with 530 additions and 442 deletions.
35 changes: 24 additions & 11 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -1142,20 +1142,33 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

function logUpdateQueue(updateQueue: UpdateQueue<mixed>, depth) {
log(' '.repeat(depth + 1) + 'QUEUED UPDATES');
const firstUpdate = updateQueue.firstUpdate;
if (!firstUpdate) {
const last = updateQueue.baseQueue;
if (last === null) {
return;
}
const first = last.next;
let update = first;
if (update !== null) {
do {
log(
' '.repeat(depth + 1) + '~',
'[' + update.expirationTime + ']',
);
} while (update !== null && update !== first);
}

log(
' '.repeat(depth + 1) + '~',
'[' + firstUpdate.expirationTime + ']',
);
while (firstUpdate.next) {
log(
' '.repeat(depth + 1) + '~',
'[' + firstUpdate.expirationTime + ']',
);
const lastPending = updateQueue.shared.pending;
if (lastPending !== null) {
const firstPending = lastPending.next;
let pendingUpdate = firstPending;
if (pendingUpdate !== null) {
do {
log(
' '.repeat(depth + 1) + '~',
'[' + pendingUpdate.expirationTime + ']',
);
} while (pendingUpdate !== null && pendingUpdate !== firstPending);
}
}
}

Expand Down
19 changes: 10 additions & 9 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,11 @@ import {
reconcileChildFibers,
cloneChildFibers,
} from './ReactChildFiber';
import {processUpdateQueue} from './ReactUpdateQueue';
import {
processUpdateQueue,
cloneUpdateQueue,
initializeUpdateQueue,
} from './ReactUpdateQueue';
import {
NoWork,
Never,
Expand Down Expand Up @@ -904,21 +908,16 @@ function updateHostRoot(current, workInProgress, renderExpirationTime) {
pushHostRootContext(workInProgress);
const updateQueue = workInProgress.updateQueue;
invariant(
updateQueue !== null,
current !== null && updateQueue !== null,
'If the root does not have an updateQueue, we should have already ' +
'bailed out. This error is likely caused by a bug in React. Please ' +
'file an issue.',
);
const nextProps = workInProgress.pendingProps;
const prevState = workInProgress.memoizedState;
const prevChildren = prevState !== null ? prevState.element : null;
processUpdateQueue(
workInProgress,
updateQueue,
nextProps,
null,
renderExpirationTime,
);
cloneUpdateQueue(current, workInProgress);
processUpdateQueue(workInProgress, nextProps, null, renderExpirationTime);
const nextState = workInProgress.memoizedState;
// Caution: React DevTools currently depends on this property
// being called "element".
Expand Down Expand Up @@ -1338,6 +1337,8 @@ function mountIndeterminateComponent(
workInProgress.memoizedState =
value.state !== null && value.state !== undefined ? value.state : null;

initializeUpdateQueue(workInProgress);

const getDerivedStateFromProps = Component.getDerivedStateFromProps;
if (typeof getDerivedStateFromProps === 'function') {
applyDerivedStateFromProps(
Expand Down
69 changes: 23 additions & 46 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {UpdateQueue} from './ReactUpdateQueue';

import React from 'react';
import {Update, Snapshot} from 'shared/ReactSideEffectTags';
Expand Down Expand Up @@ -38,6 +39,8 @@ import {
createUpdate,
ReplaceState,
ForceUpdate,
initializeUpdateQueue,
cloneUpdateQueue,
} from './ReactUpdateQueue';
import {NoWork} from './ReactFiberExpirationTime';
import {
Expand Down Expand Up @@ -171,8 +174,9 @@ export function applyDerivedStateFromProps(

// Once the update queue is empty, persist the derived state onto the
// base state.
const updateQueue = workInProgress.updateQueue;
if (updateQueue !== null && workInProgress.expirationTime === NoWork) {
if (workInProgress.expirationTime === NoWork) {
// Queue is always non-null for classes
const updateQueue: UpdateQueue<any> = (workInProgress.updateQueue: any);
updateQueue.baseState = memoizedState;
}
}
Expand Down Expand Up @@ -789,6 +793,8 @@ function mountClassInstance(
instance.state = workInProgress.memoizedState;
instance.refs = emptyRefsObject;

initializeUpdateQueue(workInProgress);

const contextType = ctor.contextType;
if (typeof contextType === 'object' && contextType !== null) {
instance.context = readContext(contextType);
Expand Down Expand Up @@ -829,17 +835,8 @@ function mountClassInstance(
}
}

let updateQueue = workInProgress.updateQueue;
if (updateQueue !== null) {
processUpdateQueue(
workInProgress,
updateQueue,
newProps,
instance,
renderExpirationTime,
);
instance.state = workInProgress.memoizedState;
}
processUpdateQueue(workInProgress, newProps, instance, renderExpirationTime);
instance.state = workInProgress.memoizedState;

const getDerivedStateFromProps = ctor.getDerivedStateFromProps;
if (typeof getDerivedStateFromProps === 'function') {
Expand All @@ -863,17 +860,13 @@ function mountClassInstance(
callComponentWillMount(workInProgress, instance);
// If we had additional state updates during this life-cycle, let's
// process them now.
updateQueue = workInProgress.updateQueue;
if (updateQueue !== null) {
processUpdateQueue(
workInProgress,
updateQueue,
newProps,
instance,
renderExpirationTime,
);
instance.state = workInProgress.memoizedState;
}
processUpdateQueue(
workInProgress,
newProps,
instance,
renderExpirationTime,
);
instance.state = workInProgress.memoizedState;
}

if (typeof instance.componentDidMount === 'function') {
Expand Down Expand Up @@ -936,17 +929,8 @@ function resumeMountClassInstance(

const oldState = workInProgress.memoizedState;
let newState = (instance.state = oldState);
let updateQueue = workInProgress.updateQueue;
if (updateQueue !== null) {
processUpdateQueue(
workInProgress,
updateQueue,
newProps,
instance,
renderExpirationTime,
);
newState = workInProgress.memoizedState;
}
processUpdateQueue(workInProgress, newProps, instance, renderExpirationTime);
newState = workInProgress.memoizedState;
if (
oldProps === newProps &&
oldState === newState &&
Expand Down Expand Up @@ -1035,6 +1019,8 @@ function updateClassInstance(
): boolean {
const instance = workInProgress.stateNode;

cloneUpdateQueue(current, workInProgress);

const oldProps = workInProgress.memoizedProps;
instance.props =
workInProgress.type === workInProgress.elementType
Expand Down Expand Up @@ -1081,17 +1067,8 @@ function updateClassInstance(

const oldState = workInProgress.memoizedState;
let newState = (instance.state = oldState);
let updateQueue = workInProgress.updateQueue;
if (updateQueue !== null) {
processUpdateQueue(
workInProgress,
updateQueue,
newProps,
instance,
renderExpirationTime,
);
newState = workInProgress.memoizedState;
}
processUpdateQueue(workInProgress, newProps, instance, renderExpirationTime);
newState = workInProgress.memoizedState;

if (
oldProps === newProps &&
Expand Down
Loading

0 comments on commit 7bf40e1

Please sign in to comment.