Skip to content

Commit

Permalink
Decouple update queue from Fiber type (#12600)
Browse files Browse the repository at this point in the history 
* Decouple update queue from Fiber type

The update queue is in need of a refactor. Recent bugfixes (#12528) have
exposed some flaws in how it's modeled. Upcoming features like Suspense
and [redacted] also rely on the update queue in ways that weren't
anticipated in the original design.

Major changes:

- Instead of boolean flags for `isReplace` and `isForceUpdate`, updates
have a `tag` field (like Fiber). This lowers the cost for adding new
types of updates.
- Render phase updates are special cased. Updates scheduled during
the render phase are dropped if the work-in-progress does not commit.
This is used for `getDerivedStateFrom{Props,Catch}`.
- `callbackList` has been replaced with a generic effect list. Aside
from callbacks, this is also used for `componentDidCatch`.

* Remove first class UpdateQueue types and use closures instead

I tried to avoid this at first, since we avoid it everywhere else in the Fiber
codebase, but since updates are not in a hot path, the trade off with file size
seems worth it.

* Store captured errors on a separate part of the update queue

This way they can be reused independently of updates like
getDerivedStateFromProps. This will be important for resuming.

* Revert back to storing hasForceUpdate on the update queue

Instead of using the effect tag. Ideally, this would be part of the
return type of processUpdateQueue.

* Rename UpdateQueue effect type back to Callback

I don't love this name either, but it's less confusing than UpdateQueue
I suppose. Conceptually, this is usually a callback: setState callbacks,
componentDidCatch. The only case that feels a bit weird is Timeouts,
which use this effect to attach a promise listener. I guess that kinda
fits, too.

* Call getDerivedStateFromProps every render, even if props did not change

Rather than enqueue a new setState updater for every props change, we
can skip the update queue entirely and merge the result into state at
the end. This makes more sense, since "receiving props" is not an event
that should be observed. It's still a bit weird, since eventually we do
persist the derived state (in other words, it accumulates).

* Store captured effects on separate list from "own" effects (callbacks)

For resuming, we need the ability to discard the "own" effects while
reusing the captured effects.

* Optimize for class components

Change `process` and `callback` to match the expected payload types
for class components. I had intended for the update queue to be reusable
for both class components and a future React API, but we'll likely have
to fork anyway.

* Only double-invoke render phase lifecycles functions in DEV

* Use global state to track currently processing queue in DEV
  • Loading branch information
@acdlite
acdlite authored Apr 23, 2018
1 parent 5dcf93d commit b548b3c
Show file tree
Hide file tree
Showing 17 changed files with 1,112 additions and 1,005 deletions.
14 changes: 4 additions & 10 deletions packages/create-subscription/src/__tests__/createSubscription-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,6 @@ describe('createSubscription', () => {

it('should ignore values emitted by a new subscribable until the commit phase', () => {
const log = [];
let parentInstance;

function Child({value}) {
ReactNoop.yield('Child: ' + value);
Expand Down Expand Up @@ -301,8 +300,6 @@ describe('createSubscription', () => {
}

render() {
parentInstance = this;

return (
<Subscription source={this.state.observed}>
{(value = 'default') => {
Expand Down Expand Up @@ -331,8 +328,8 @@ describe('createSubscription', () => {
observableB.next('b-2');
observableB.next('b-3');

// Mimic a higher-priority interruption
parentInstance.setState({observed: observableA});
// Update again
ReactNoop.render(<Parent observed={observableA} />);

// Flush everything and ensure that the correct subscribable is used
// We expect the last emitted update to be rendered (because of the commit phase value check)
Expand All @@ -354,7 +351,6 @@ describe('createSubscription', () => {

it('should not drop values emitted between updates', () => {
const log = [];
let parentInstance;

function Child({value}) {
ReactNoop.yield('Child: ' + value);
Expand Down Expand Up @@ -391,8 +387,6 @@ describe('createSubscription', () => {
}

render() {
parentInstance = this;

return (
<Subscription source={this.state.observed}>
{(value = 'default') => {
Expand Down Expand Up @@ -420,8 +414,8 @@ describe('createSubscription', () => {
observableA.next('a-1');
observableA.next('a-2');

// Mimic a higher-priority interruption
parentInstance.setState({observed: observableA});
// Update again
ReactNoop.render(<Parent observed={observableA} />);

// Flush everything and ensure that the correct subscribable is used
// We expect the new subscribable to finish rendering,
Expand Down
16 changes: 4 additions & 12 deletions packages/react-noop-renderer/src/ReactNoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import type {Fiber} from 'react-reconciler/src/ReactFiber';
import type {UpdateQueue} from 'react-reconciler/src/ReactFiberUpdateQueue';
import type {UpdateQueue} from 'react-reconciler/src/ReactUpdateQueue';
import type {ReactNodeList} from 'shared/ReactTypes';
import ReactFiberReconciler from 'react-reconciler';
import {enablePersistentReconciler} from 'shared/ReactFeatureFlags';
Expand Down Expand Up @@ -526,23 +526,15 @@ const ReactNoop = {

function logUpdateQueue(updateQueue: UpdateQueue<mixed>, depth) {
log(' '.repeat(depth + 1) + 'QUEUED UPDATES');
const firstUpdate = updateQueue.first;
const firstUpdate = updateQueue.firstUpdate;
if (!firstUpdate) {
return;
}

log(
' '.repeat(depth + 1) + '~',
firstUpdate && firstUpdate.partialState,
firstUpdate.callback ? 'with callback' : '',
'[' + firstUpdate.expirationTime + ']',
);
let next;
while ((next = firstUpdate.next)) {
log(' '.repeat(depth + 1) + '~', '[' + firstUpdate.expirationTime + ']');
while (firstUpdate.next) {
log(
' '.repeat(depth + 1) + '~',
next.partialState,
next.callback ? 'with callback' : '',
'[' + firstUpdate.expirationTime + ']',
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiber.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import type {TypeOfWork} from 'shared/ReactTypeOfWork';
import type {TypeOfMode} from './ReactTypeOfMode';
import type {TypeOfSideEffect} from 'shared/ReactTypeOfSideEffect';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {UpdateQueue} from './ReactFiberUpdateQueue';
import type {UpdateQueue} from './ReactUpdateQueue';

import invariant from 'fbjs/lib/invariant';
import {NoEffect} from 'shared/ReactTypeOfSideEffect';
Expand Down
80 changes: 28 additions & 52 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ import {
ContextConsumer,
} from 'shared/ReactTypeOfWork';
import {
NoEffect,
PerformedWork,
Placement,
ContentReset,
Ref,
DidCapture,
} from 'shared/ReactTypeOfSideEffect';
import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState';
import {
Expand All @@ -53,13 +55,15 @@ import warning from 'fbjs/lib/warning';
import ReactDebugCurrentFiber from './ReactDebugCurrentFiber';
import {cancelWorkTimer} from './ReactDebugFiberPerf';

import ReactFiberClassComponent from './ReactFiberClassComponent';
import ReactFiberClassComponent, {
applyDerivedStateFromProps,
} from './ReactFiberClassComponent';
import {
mountChildFibers,
reconcileChildFibers,
cloneChildFibers,
} from './ReactChildFiber';
import {processUpdateQueue} from './ReactFiberUpdateQueue';
import {processUpdateQueue} from './ReactUpdateQueue';
import {NoWork, Never} from './ReactFiberExpirationTime';
import {AsyncMode, StrictMode} from './ReactTypeOfMode';
import MAX_SIGNED_31_BIT_INT from './maxSigned31BitInt';
Expand Down Expand Up @@ -108,7 +112,6 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(

const {
adoptClassInstance,
callGetDerivedStateFromProps,
constructClassInstance,
mountClassInstance,
resumeMountClassInstance,
Expand Down Expand Up @@ -263,7 +266,11 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
if (current === null) {
if (workInProgress.stateNode === null) {
// In the initial pass we might need to construct the instance.
constructClassInstance(workInProgress, workInProgress.pendingProps);
constructClassInstance(
workInProgress,
workInProgress.pendingProps,
renderExpirationTime,
);
mountClassInstance(workInProgress, renderExpirationTime);

shouldUpdate = true;
Expand All @@ -281,22 +288,11 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
renderExpirationTime,
);
}

// We processed the update queue inside updateClassInstance. It may have
// included some errors that were dispatched during the commit phase.
// TODO: Refactor class components so this is less awkward.
let didCaptureError = false;
const updateQueue = workInProgress.updateQueue;
if (updateQueue !== null && updateQueue.capturedValues !== null) {
shouldUpdate = true;
didCaptureError = true;
}
return finishClassComponent(
current,
workInProgress,
shouldUpdate,
hasContext,
didCaptureError,
renderExpirationTime,
);
}
Expand All @@ -306,12 +302,14 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
workInProgress: Fiber,
shouldUpdate: boolean,
hasContext: boolean,
didCaptureError: boolean,
renderExpirationTime: ExpirationTime,
) {
// Refs should update even if shouldComponentUpdate returns false
markRef(current, workInProgress);

const didCaptureError =
(workInProgress.effectTag & DidCapture) !== NoEffect;

if (!shouldUpdate && !didCaptureError) {
// Context providers should defer to sCU for rendering
if (hasContext) {
Expand Down Expand Up @@ -351,13 +349,6 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
}
ReactDebugCurrentFiber.setCurrentPhase(null);
} else {
if (
debugRenderPhaseSideEffects ||
(debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictMode)
) {
instance.render();
}
nextChildren = instance.render();
}
}
Expand Down Expand Up @@ -416,29 +407,24 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
pushHostRootContext(workInProgress);
let updateQueue = workInProgress.updateQueue;
if (updateQueue !== null) {
const nextProps = workInProgress.pendingProps;
const prevState = workInProgress.memoizedState;
const state = processUpdateQueue(
current,
const prevChildren = prevState !== null ? prevState.children : null;
processUpdateQueue(
workInProgress,
updateQueue,
null,
nextProps,
null,
renderExpirationTime,
);
memoizeState(workInProgress, state);
updateQueue = workInProgress.updateQueue;

let element;
if (updateQueue !== null && updateQueue.capturedValues !== null) {
// There's an uncaught error. Unmount the whole root.
element = null;
} else if (prevState === state) {
const nextState = workInProgress.memoizedState;
const nextChildren = nextState.children;

if (nextChildren === prevChildren) {
// If the state is the same as before, that's a bailout because we had
// no work that expires at this time.
resetHydrationState();
return bailoutOnAlreadyFinishedWork(current, workInProgress);
} else {
element = state.element;
}
const root: FiberRoot = workInProgress.stateNode;
if (
Expand All @@ -463,16 +449,15 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
workInProgress.child = mountChildFibers(
workInProgress,
null,
element,
nextChildren,
renderExpirationTime,
);
} else {
// Otherwise reset hydration state in case we aborted and resumed another
// root.
resetHydrationState();
reconcileChildren(current, workInProgress, element);
reconcileChildren(current, workInProgress, nextChildren);
}
memoizeState(workInProgress, state);
return workInProgress.child;
}
resetHydrationState();
Expand Down Expand Up @@ -610,21 +595,13 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
workInProgress.memoizedState =
value.state !== null && value.state !== undefined ? value.state : null;

if (typeof Component.getDerivedStateFromProps === 'function') {
const partialState = callGetDerivedStateFromProps(
const getDerivedStateFromProps = Component.getDerivedStateFromProps;
if (typeof getDerivedStateFromProps === 'function') {
applyDerivedStateFromProps(
workInProgress,
value,
getDerivedStateFromProps,
props,
workInProgress.memoizedState,
);

if (partialState !== null && partialState !== undefined) {
workInProgress.memoizedState = Object.assign(
{},
workInProgress.memoizedState,
partialState,
);
}
}

// Push context providers early to prevent context stack mismatches.
Expand All @@ -638,7 +615,6 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
workInProgress,
true,
hasContext,
false,
renderExpirationTime,
);
} else {
Expand Down
Loading

0 comments on commit b548b3c

Please sign in to comment.