From 4ff736f2ebedefdabcb4ad1986d6a5722cfac4b3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 22 Dec 2016 14:57:05 +0000 Subject: [PATCH 1/7] Push class context providers early Previously we used to push them only after the instance was available. This caused issues in cases an error is thrown during componentWillMount(). In that case we never got to pushing the provider in the begin phase, but in complete phase the provider check returned true since the instance existed by that point. As a result we got mismatching context pops. We solve the issue by making the context check independent of whether the instance actually exists. Instead we're checking the type itself. This lets us push class context early. However there's another problem: we might not know the context value. If the instance is not yet created, we can't call getChildContext on it. To fix this, we are introducing a way to replace current value on the stack, and a way to read the previous value. This also helps remove some branching and split the memoized from invalidated code paths. --- scripts/fiber/tests-passing.txt | 2 + .../shared/fiber/ReactFiberBeginWork.js | 43 +++++++++---- .../shared/fiber/ReactFiberContext.js | 60 +++++++++++-------- src/renderers/shared/fiber/ReactFiberStack.js | 36 +++++++++++ .../__tests__/ReactErrorBoundaries-test.js | 52 ++++++++++++++++ 5 files changed, 156 insertions(+), 37 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 40d2350c562d..aa4099d6447f 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1413,6 +1413,8 @@ src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js * renders an error state if child throws in render * renders an error state if child throws in constructor * renders an error state if child throws in componentWillMount +* renders an error state if context provider throws in componentWillMount +* renders an error state if module-style context provider throws in componentWillMount * mounts the error message if mounting fails * propagates errors on retry on mounting * propagates errors inside boundary during componentWillMount diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index ce503be9ec94..130115ece638 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -35,6 +35,7 @@ var { hasContextChanged, pushContextProvider, pushTopLevelContextObject, + invalidateContextProvider, } = require('ReactFiberContext'); var { IndeterminateComponent, @@ -215,6 +216,14 @@ module.exports = function( } function updateClassComponent(current : ?Fiber, workInProgress : Fiber, priorityLevel : PriorityLevel) { + // Push context providers early to prevent context stack mismatches. + // During mounting we don't know the child context yet as the instance doesn't exist. + // We will invalidate the child context in finishClassComponent() right after rendering. + const hasContext = isContextProvider(workInProgress); + if (hasContext) { + pushContextProvider(workInProgress); + } + let shouldUpdate; if (!current) { if (!workInProgress.stateNode) { @@ -229,10 +238,15 @@ module.exports = function( } else { shouldUpdate = updateClassInstance(current, workInProgress, priorityLevel); } - return finishClassComponent(current, workInProgress, shouldUpdate); + return finishClassComponent(current, workInProgress, shouldUpdate, hasContext); } - function finishClassComponent(current : ?Fiber, workInProgress : Fiber, shouldUpdate : boolean) { + function finishClassComponent( + current : ?Fiber, + workInProgress : Fiber, + shouldUpdate : boolean, + hasContext : boolean, + ) { // Schedule side-effects if (shouldUpdate) { workInProgress.effectTag |= Update; @@ -246,11 +260,6 @@ module.exports = function( workInProgress.effectTag |= Update; } } - - // Don't forget to push the context before returning. - if (isContextProvider(workInProgress)) { - pushContextProvider(workInProgress, false); - } return bailoutOnAlreadyFinishedWork(current, workInProgress); } @@ -259,9 +268,10 @@ module.exports = function( ReactCurrentOwner.current = workInProgress; const nextChildren = instance.render(); reconcileChildren(current, workInProgress, nextChildren); - // Put context on the stack because we will work on children - if (isContextProvider(workInProgress)) { - pushContextProvider(workInProgress, true); + + // The context might have changed so we need to recalculate it. + if (hasContext) { + invalidateContextProvider(workInProgress); } return workInProgress.child; } @@ -417,9 +427,18 @@ module.exports = function( if (typeof value === 'object' && value && typeof value.render === 'function') { // Proceed under the assumption that this is a class instance workInProgress.tag = ClassComponent; + + // Push context providers early to prevent context stack mismatches. + // During mounting we don't know the child context yet as the instance doesn't exist. + // We will invalidate the child context in finishClassComponent() right after rendering. + const hasContext = isContextProvider(workInProgress); + if (hasContext) { + pushContextProvider(workInProgress); + } + adoptClassInstance(workInProgress, value); mountClassInstance(workInProgress, priorityLevel); - return finishClassComponent(current, workInProgress, true); + return finishClassComponent(current, workInProgress, true, hasContext); } else { // Proceed under the assumption that this is a functional component workInProgress.tag = FunctionalComponent; @@ -536,7 +555,7 @@ module.exports = function( switch (workInProgress.tag) { case ClassComponent: if (isContextProvider(workInProgress)) { - pushContextProvider(workInProgress, false); + pushContextProvider(workInProgress); } break; case HostPortal: diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 013a083f8cb8..01ce7081c03a 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -27,8 +27,10 @@ var { } = require('ReactTypeOfWork'); const { createCursor, + getPrevious, pop, push, + replace, } = require('ReactFiberStack'); if (__DEV__) { @@ -38,10 +40,6 @@ if (__DEV__) { let contextStackCursor : StackCursor = createCursor((null: ?Object)); let didPerformWorkStackCursor : StackCursor = createCursor(false); -function getUnmaskedContext() { - return contextStackCursor.current || emptyObject; -} - exports.getMaskedContext = function(workInProgress : Fiber) { const type = workInProgress.type; const contextTypes = type.contextTypes; @@ -49,11 +47,17 @@ exports.getMaskedContext = function(workInProgress : Fiber) { return emptyObject; } - const unmaskedContext = getUnmaskedContext(); - const context = {}; + const hasOwnContext = isContextProvider(workInProgress); + // If it is a context provider then use the previous context instead. + const unmaskedContext = hasOwnContext ? + getPrevious(contextStackCursor) : + contextStackCursor.current; - for (let key in contextTypes) { - context[key] = unmaskedContext[key]; + const context = {}; + if (unmaskedContext != null) { + for (let key in contextTypes) { + context[key] = unmaskedContext[key]; + } } if (__DEV__) { @@ -71,9 +75,7 @@ exports.hasContextChanged = function() : boolean { function isContextProvider(fiber : Fiber) : boolean { return ( fiber.tag === ClassComponent && - // Instance might be null, if the fiber errored during construction - fiber.stateNode && - typeof fiber.stateNode.getChildContext === 'function' + fiber.type.childContextTypes != null ); } exports.isContextProvider = isContextProvider; @@ -91,7 +93,7 @@ exports.pushTopLevelContextObject = function(fiber : Fiber, context : Object, di push(didPerformWorkStackCursor, didChange, fiber); }; -function processChildContext(fiber : Fiber, parentContext : Object, isReconciling : boolean): Object { +function processChildContext(fiber : Fiber, parentContext : ?Object, isReconciling : boolean): Object { const instance = fiber.stateNode; const childContextTypes = fiber.type.childContextTypes; const childContext = instance.getChildContext(); @@ -117,21 +119,29 @@ function processChildContext(fiber : Fiber, parentContext : Object, isReconcilin } exports.processChildContext = processChildContext; -exports.pushContextProvider = function(workInProgress : Fiber, didPerformWork : boolean) : void { +exports.pushContextProvider = function(workInProgress : Fiber) : void { const instance = workInProgress.stateNode; - const memoizedMergedChildContext = instance.__reactInternalMemoizedMergedChildContext; - const canReuseMergedChildContext = !didPerformWork && memoizedMergedChildContext != null; - - let mergedContext = null; - if (canReuseMergedChildContext) { - mergedContext = memoizedMergedChildContext; - } else { - mergedContext = processChildContext(workInProgress, getUnmaskedContext(), true); - instance.__reactInternalMemoizedMergedChildContext = mergedContext; - } + // We push the context as early as possible to ensure stack integrity. + // If the instance does not exist yet, we will push null at first, + // and replace it on the stack later when invalidating the context. + const memoizedMergedChildContext = ( + instance && + instance.__reactInternalMemoizedMergedChildContext + ) || null; + push(contextStackCursor, memoizedMergedChildContext, workInProgress); + push(didPerformWorkStackCursor, false, workInProgress); +}; - push(contextStackCursor, mergedContext, workInProgress); - push(didPerformWorkStackCursor, didPerformWork, workInProgress); +exports.invalidateContextProvider = function(workInProgress : Fiber) : void { + const instance = workInProgress.stateNode; + if (instance == null) { + throw new Error('Expected to have an instance by this point.'); + } + const parentContext = getPrevious(contextStackCursor); + const mergedContext = processChildContext(workInProgress, parentContext, true); + instance.__reactInternalMemoizedMergedChildContext = mergedContext; + replace(contextStackCursor, mergedContext, workInProgress); + replace(didPerformWorkStackCursor, true, workInProgress); }; exports.resetContext = function() : void { diff --git a/src/renderers/shared/fiber/ReactFiberStack.js b/src/renderers/shared/fiber/ReactFiberStack.js index 3d51b4aec64c..6fa253462dfa 100644 --- a/src/renderers/shared/fiber/ReactFiberStack.js +++ b/src/renderers/shared/fiber/ReactFiberStack.js @@ -34,6 +34,21 @@ exports.createCursor = function(defaultValue : T) : StackCursor { }; }; +exports.getPrevious = function( + cursor : StackCursor, +) : T | null { + if (index < 0) { + if (__DEV__) { + warning(false, 'Unexpected read of previous.'); + } + return null; + } + if (index === 0) { + return null; + } + return valueStack[index - 1]; +}; + exports.isEmpty = function() : boolean { return index === -1; }; @@ -82,6 +97,27 @@ exports.push = function( cursor.current = value; }; +exports.replace = function( + cursor : StackCursor, + value : T, + fiber: Fiber, +) : void { + if (index < 0) { + if (__DEV__) { + warning(false, 'Unexpected replace.'); + } + return; + } + + if (__DEV__) { + if (fiber !== fiberStack[index]) { + warning(false, 'Unexpected Fiber replaced.'); + } + } + + cursor.current = value; +}; + exports.reset = function() : void { while (index > -1) { valueStack[index] = null; diff --git a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js index 6ec4e3697ab6..f075432d74a4 100644 --- a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js @@ -733,6 +733,58 @@ describe('ReactErrorBoundaries', () => { ]); }); + it('renders an error state if context provider throws in componentWillMount', () => { + class BrokenComponentWillMountWithContext extends React.Component { + static childContextTypes = {foo: React.PropTypes.number}; + getChildContext() { + return {foo: 42}; + } + render() { + return
{this.props.children}
; + } + componentWillMount() { + throw new Error('Hello'); + } + } + + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + }); + + it('renders an error state if module-style context provider throws in componentWillMount', () => { + function BrokenComponentWillMountWithContext() { + return { + getChildContext() { + return {foo: 42}; + }, + render() { + return
{this.props.children}
; + }, + componentWillMount() { + throw new Error('Hello'); + }, + }; + } + BrokenComponentWillMountWithContext.childContextTypes = { + foo: React.PropTypes.number, + }; + + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + }); + it('mounts the error message if mounting fails', () => { function renderError(error) { return ( From d64c76cf52b944f467a3aea02efc85e779505cce Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 22 Dec 2016 15:04:42 +0000 Subject: [PATCH 2/7] Add a now-passing test from #8604 Also rename another test to have a shorter name. --- scripts/fiber/tests-passing.txt | 3 +- .../fiber/__tests__/ReactIncremental-test.js | 49 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index aa4099d6447f..cc1e1ff46488 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1157,7 +1157,8 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * provides context when reusing work * reads context when setState is below the provider * reads context when setState is above the provider -* maintains the correct context index when context proviers are bailed out due to low priority +* maintains the correct context when providers bail out due to low priority +* maintains the correct context when unwinding due to an error in render src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * catches render error in a boundary during full deferred mounting diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index df98cca8ad79..defd5e672b23 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -1932,7 +1932,7 @@ describe('ReactIncremental', () => { ]); }); - it('maintains the correct context index when context proviers are bailed out due to low priority', () => { + it('maintains the correct context when providers bail out due to low priority', () => { class Root extends React.Component { render() { return ; @@ -1974,4 +1974,51 @@ describe('ReactIncremental', () => { instance.setState({}); ReactNoop.flush(); }); + + it('maintains the correct context when unwinding due to an error in render', () => { + class Root extends React.Component { + unstable_handleError(error) { + // If context is pushed/popped correctly, + // This method will be used to handle the intentionally-thrown Error. + } + render() { + return ; + } + } + + let instance; + + class ContextProvider extends React.Component { + constructor(props, context) { + super(props, context); + this.state = {}; + if (props.depth === 1) { + instance = this; + } + } + static childContextTypes = {}; + getChildContext() { + return {}; + } + render() { + if (this.state.throwError) { + throw Error(); + } + return this.props.depth < 4 + ? + :
; + } + } + + // Init + ReactNoop.render(); + ReactNoop.flush(); + + // Trigger an update in the middle of the tree + // This is necessary to reproduce the error as it curently exists. + instance.setState({ + throwError: true, + }); + ReactNoop.flush(); + }); }); From c57cf3ef6f18960026b18c7d5981259d89060f7e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 22 Dec 2016 15:21:38 +0000 Subject: [PATCH 3/7] Move isContextProvider() checks into push() and pop() All uses of push() and pop() are guarded by it anyway. This makes it more similar to how we use host context. There is only one other place where isContextProvider() is used and that's legacy code needed for renderSubtree(). --- .../shared/fiber/ReactFiberBeginWork.js | 16 +++------------- .../shared/fiber/ReactFiberCompleteWork.js | 5 +---- src/renderers/shared/fiber/ReactFiberContext.js | 10 +++++++++- .../shared/fiber/ReactFiberScheduler.js | 5 +---- 4 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 130115ece638..72a6a1f8bcdc 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -31,7 +31,6 @@ var { var ReactTypeOfWork = require('ReactTypeOfWork'); var { getMaskedContext, - isContextProvider, hasContextChanged, pushContextProvider, pushTopLevelContextObject, @@ -219,10 +218,7 @@ module.exports = function( // Push context providers early to prevent context stack mismatches. // During mounting we don't know the child context yet as the instance doesn't exist. // We will invalidate the child context in finishClassComponent() right after rendering. - const hasContext = isContextProvider(workInProgress); - if (hasContext) { - pushContextProvider(workInProgress); - } + const hasContext = pushContextProvider(workInProgress); let shouldUpdate; if (!current) { @@ -431,11 +427,7 @@ module.exports = function( // Push context providers early to prevent context stack mismatches. // During mounting we don't know the child context yet as the instance doesn't exist. // We will invalidate the child context in finishClassComponent() right after rendering. - const hasContext = isContextProvider(workInProgress); - if (hasContext) { - pushContextProvider(workInProgress); - } - + const hasContext = pushContextProvider(workInProgress); adoptClassInstance(workInProgress, value); mountClassInstance(workInProgress, priorityLevel); return finishClassComponent(current, workInProgress, true, hasContext); @@ -554,9 +546,7 @@ module.exports = function( // See PR 8590 discussion for context switch (workInProgress.tag) { case ClassComponent: - if (isContextProvider(workInProgress)) { - pushContextProvider(workInProgress); - } + pushContextProvider(workInProgress); break; case HostPortal: pushHostContainer(workInProgress, workInProgress.stateNode.containerInfo); diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index f58a689d1441..c0f41ab4c872 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -21,7 +21,6 @@ import type { ReifiedYield } from 'ReactReifiedYield'; var { reconcileChildFibers } = require('ReactChildFiber'); var { - isContextProvider, popContextProvider, } = require('ReactFiberContext'); var ReactTypeOfWork = require('ReactTypeOfWork'); @@ -175,9 +174,7 @@ module.exports = function( return null; case ClassComponent: { // We are leaving this subtree, so pop context if any. - if (isContextProvider(workInProgress)) { - popContextProvider(workInProgress); - } + popContextProvider(workInProgress); // Don't use the state queue to compute the memoized state. We already // merged it and assigned it to the instance. Transfer it from there. // Also need to transfer the props, because pendingProps will be null diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 01ce7081c03a..ed849db3781f 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -81,6 +81,9 @@ function isContextProvider(fiber : Fiber) : boolean { exports.isContextProvider = isContextProvider; function popContextProvider(fiber : Fiber) : void { + if (!isContextProvider(fiber)) { + return; + } pop(didPerformWorkStackCursor, fiber); pop(contextStackCursor, fiber); } @@ -119,7 +122,11 @@ function processChildContext(fiber : Fiber, parentContext : ?Object, isReconcili } exports.processChildContext = processChildContext; -exports.pushContextProvider = function(workInProgress : Fiber) : void { +exports.pushContextProvider = function(workInProgress : Fiber) : boolean { + if (!isContextProvider(workInProgress)) { + return false; + } + const instance = workInProgress.stateNode; // We push the context as early as possible to ensure stack integrity. // If the instance does not exist yet, we will push null at first, @@ -130,6 +137,7 @@ exports.pushContextProvider = function(workInProgress : Fiber) : void { ) || null; push(contextStackCursor, memoizedMergedChildContext, workInProgress); push(didPerformWorkStackCursor, false, workInProgress); + return true; }; exports.invalidateContextProvider = function(workInProgress : Fiber) : void { diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 2e7635cfe764..5f203409f9e5 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -18,7 +18,6 @@ import type { HostConfig, Deadline } from 'ReactFiberReconciler'; import type { PriorityLevel } from 'ReactPriorityLevel'; var { - isContextProvider, popContextProvider, } = require('ReactFiberContext'); const { reset } = require('ReactFiberStack'); @@ -957,9 +956,7 @@ module.exports = function(config : HostConfig Date: Thu, 22 Dec 2016 17:02:46 +0000 Subject: [PATCH 4/7] Clarify why we read the previous context --- src/renderers/shared/fiber/ReactFiberContext.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index ed849db3781f..9216bd8ce9e1 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -48,7 +48,10 @@ exports.getMaskedContext = function(workInProgress : Fiber) { } const hasOwnContext = isContextProvider(workInProgress); - // If it is a context provider then use the previous context instead. + // If the fiber is a context provider itself, by the time we read its context + // we have already pushed its own child context on the stack. A context + // provider should not "see" its own child context. Therefore we read the + // previous (parent) context instead for context providers. const unmaskedContext = hasOwnContext ? getPrevious(contextStackCursor) : contextStackCursor.current; From a6f5cf0bf54f9cf80fcf225d0cc3fa144a02f45f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 22 Dec 2016 17:03:39 +0000 Subject: [PATCH 5/7] Use invariant instead of throwing an error --- src/renderers/shared/fiber/ReactFiberContext.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 9216bd8ce9e1..16609ea18e85 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -145,9 +145,7 @@ exports.pushContextProvider = function(workInProgress : Fiber) : boolean { exports.invalidateContextProvider = function(workInProgress : Fiber) : void { const instance = workInProgress.stateNode; - if (instance == null) { - throw new Error('Expected to have an instance by this point.'); - } + invariant(instance, 'Expected to have an instance by this point.'); const parentContext = getPrevious(contextStackCursor); const mergedContext = processChildContext(workInProgress, parentContext, true); instance.__reactInternalMemoizedMergedChildContext = mergedContext; From 504a69497a40a099bf17775f35db514c7ea89913 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 22 Dec 2016 17:06:19 +0000 Subject: [PATCH 6/7] Fix off-by-one in ReactFiberStack --- src/renderers/shared/fiber/ReactFiberStack.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberStack.js b/src/renderers/shared/fiber/ReactFiberStack.js index 6fa253462dfa..e7038856dc34 100644 --- a/src/renderers/shared/fiber/ReactFiberStack.js +++ b/src/renderers/shared/fiber/ReactFiberStack.js @@ -43,10 +43,7 @@ exports.getPrevious = function( } return null; } - if (index === 0) { - return null; - } - return valueStack[index - 1]; + return valueStack[index]; }; exports.isEmpty = function() : boolean { From 59a9b6ecfa3c6ab6a2caad8daf687fd5027047d8 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 22 Dec 2016 18:19:17 +0000 Subject: [PATCH 7/7] Manually keep track of the last parent context The previous algorithm was flawed and worked by accident, as shown by the failing tests after an off-by-one was fixed. The implementation of getPrevious() was incorrect because the context stack currently has no notion of a previous value per cursor. Instead, we are caching the previous value directly in the ReactFiberContext in a local variable. Additionally, we are using push() and pop() instead of adding a new replace() method. --- .../shared/fiber/ReactFiberContext.js | 65 ++++++++++++------- src/renderers/shared/fiber/ReactFiberStack.js | 33 ---------- 2 files changed, 42 insertions(+), 56 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 16609ea18e85..a073b5280d89 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -27,18 +27,34 @@ var { } = require('ReactTypeOfWork'); const { createCursor, - getPrevious, pop, push, - replace, } = require('ReactFiberStack'); if (__DEV__) { var checkReactTypeSpec = require('checkReactTypeSpec'); } -let contextStackCursor : StackCursor = createCursor((null: ?Object)); +// A cursor to the current merged context object on the stack. +let contextStackCursor : StackCursor = createCursor(emptyObject); +// A cursor to a boolean indicating whether the context has changed. let didPerformWorkStackCursor : StackCursor = createCursor(false); +// Keep track of the previous context object that was on the stack. +// We use this to get access to the parent context after we have already +// pushed the next context provider, and now need to merge their contexts. +let previousContext : Object = emptyObject; + +function getUnmaskedContext(workInProgress : Fiber) : Object { + const hasOwnContext = isContextProvider(workInProgress); + if (hasOwnContext) { + // If the fiber is a context provider itself, when we read its context + // we have already pushed its own child context on the stack. A context + // provider should not "see" its own child context. Therefore we read the + // previous (parent) context instead for a context provider. + return previousContext; + } + return contextStackCursor.current; +} exports.getMaskedContext = function(workInProgress : Fiber) { const type = workInProgress.type; @@ -47,20 +63,10 @@ exports.getMaskedContext = function(workInProgress : Fiber) { return emptyObject; } - const hasOwnContext = isContextProvider(workInProgress); - // If the fiber is a context provider itself, by the time we read its context - // we have already pushed its own child context on the stack. A context - // provider should not "see" its own child context. Therefore we read the - // previous (parent) context instead for context providers. - const unmaskedContext = hasOwnContext ? - getPrevious(contextStackCursor) : - contextStackCursor.current; - + const unmaskedContext = getUnmaskedContext(workInProgress); const context = {}; - if (unmaskedContext != null) { - for (let key in contextTypes) { - context[key] = unmaskedContext[key]; - } + for (let key in contextTypes) { + context[key] = unmaskedContext[key]; } if (__DEV__) { @@ -87,6 +93,7 @@ function popContextProvider(fiber : Fiber) : void { if (!isContextProvider(fiber)) { return; } + pop(didPerformWorkStackCursor, fiber); pop(contextStackCursor, fiber); } @@ -99,7 +106,7 @@ exports.pushTopLevelContextObject = function(fiber : Fiber, context : Object, di push(didPerformWorkStackCursor, didChange, fiber); }; -function processChildContext(fiber : Fiber, parentContext : ?Object, isReconciling : boolean): Object { +function processChildContext(fiber : Fiber, parentContext : Object, isReconciling : boolean): Object { const instance = fiber.stateNode; const childContextTypes = fiber.type.childContextTypes; const childContext = instance.getChildContext(); @@ -137,24 +144,36 @@ exports.pushContextProvider = function(workInProgress : Fiber) : boolean { const memoizedMergedChildContext = ( instance && instance.__reactInternalMemoizedMergedChildContext - ) || null; + ) || emptyObject; + + // Remember the parent context so we can merge with it later. + previousContext = contextStackCursor.current; push(contextStackCursor, memoizedMergedChildContext, workInProgress); push(didPerformWorkStackCursor, false, workInProgress); + return true; }; exports.invalidateContextProvider = function(workInProgress : Fiber) : void { const instance = workInProgress.stateNode; invariant(instance, 'Expected to have an instance by this point.'); - const parentContext = getPrevious(contextStackCursor); - const mergedContext = processChildContext(workInProgress, parentContext, true); + + // Merge parent and own context. + const mergedContext = processChildContext(workInProgress, previousContext, true); instance.__reactInternalMemoizedMergedChildContext = mergedContext; - replace(contextStackCursor, mergedContext, workInProgress); - replace(didPerformWorkStackCursor, true, workInProgress); + + // Replace the old (or empty) context with the new one. + // It is important to unwind the context in the reverse order. + pop(didPerformWorkStackCursor, workInProgress); + pop(contextStackCursor, workInProgress); + // Now push the new context and mark that it has changed. + push(contextStackCursor, mergedContext, workInProgress); + push(didPerformWorkStackCursor, true, workInProgress); }; exports.resetContext = function() : void { - contextStackCursor.current = null; + previousContext = emptyObject; + contextStackCursor.current = emptyObject; didPerformWorkStackCursor.current = false; }; diff --git a/src/renderers/shared/fiber/ReactFiberStack.js b/src/renderers/shared/fiber/ReactFiberStack.js index e7038856dc34..3d51b4aec64c 100644 --- a/src/renderers/shared/fiber/ReactFiberStack.js +++ b/src/renderers/shared/fiber/ReactFiberStack.js @@ -34,18 +34,6 @@ exports.createCursor = function(defaultValue : T) : StackCursor { }; }; -exports.getPrevious = function( - cursor : StackCursor, -) : T | null { - if (index < 0) { - if (__DEV__) { - warning(false, 'Unexpected read of previous.'); - } - return null; - } - return valueStack[index]; -}; - exports.isEmpty = function() : boolean { return index === -1; }; @@ -94,27 +82,6 @@ exports.push = function( cursor.current = value; }; -exports.replace = function( - cursor : StackCursor, - value : T, - fiber: Fiber, -) : void { - if (index < 0) { - if (__DEV__) { - warning(false, 'Unexpected replace.'); - } - return; - } - - if (__DEV__) { - if (fiber !== fiberStack[index]) { - warning(false, 'Unexpected Fiber replaced.'); - } - } - - cursor.current = value; -}; - exports.reset = function() : void { while (index > -1) { valueStack[index] = null;