From cce4d686467e180cc81afca619407b0ad5d9945d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 20 Aug 2018 14:28:53 +0100 Subject: [PATCH 1/4] Add a repro case for context bug --- .../ReactIncremental-test.internal.js | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js index 0ffd7079d73..6388796b163 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js @@ -2030,6 +2030,39 @@ describe('ReactIncremental', () => { ]); }); + it('does not leak own context into context provider (factory components)', () => { + const ops = []; + function Recurse(props, context) { + return { + getChildContext() { + return {n: (context.n || 3) - 1}; + }, + render() { + ops.push('Recurse ' + JSON.stringify(context)); + if (context.n === 0) { + return null; + } + return ; + }, + }; + } + Recurse.contextTypes = { + n: PropTypes.number, + }; + Recurse.childContextTypes = { + n: PropTypes.number, + }; + + ReactNoop.render(); + ReactNoop.flush(); + expect(ops).toEqual([ + 'Recurse {}', + 'Recurse {"n":2}', + 'Recurse {"n":1}', + 'Recurse {"n":0}', + ]); + }); + it('provides context when reusing work', () => { const ops = []; From 5843578e001a40087a4deb4c6f1c98d55348bd9b Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 20 Aug 2018 14:34:52 +0100 Subject: [PATCH 2/4] Be explicit about whether context has been pushed --- .../react-reconciler/src/ReactFiberBeginWork.js | 4 ++-- .../src/ReactFiberClassComponent.js | 16 ++++++++++++---- .../react-reconciler/src/ReactFiberContext.js | 5 +++-- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 582cefc7aaa..b0026d1393a 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -262,7 +262,7 @@ function updateFunctionalComponent( nextProps: any, renderExpirationTime, ) { - const unmaskedContext = getUnmaskedContext(workInProgress, Component); + const unmaskedContext = getUnmaskedContext(workInProgress, Component, true); const context = getMaskedContext(workInProgress, unmaskedContext); let nextChildren; @@ -674,7 +674,7 @@ function mountIndeterminateComponent( } } - const unmaskedContext = getUnmaskedContext(workInProgress, Component); + const unmaskedContext = getUnmaskedContext(workInProgress, Component, false); const context = getMaskedContext(workInProgress, unmaskedContext); prepareToReadContext(workInProgress, renderExpirationTime); diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index a4b3692a5af..34a1c5a37d2 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -473,7 +473,7 @@ function constructClassInstance( props: any, renderExpirationTime: ExpirationTime, ): any { - const unmaskedContext = getUnmaskedContext(workInProgress, ctor); + const unmaskedContext = getUnmaskedContext(workInProgress, ctor, true); const contextTypes = ctor.contextTypes; const isContextConsumer = contextTypes !== null && contextTypes !== undefined; const context = isContextConsumer @@ -666,7 +666,7 @@ function mountClassInstance( } const instance = workInProgress.stateNode; - const unmaskedContext = getUnmaskedContext(workInProgress, ctor); + const unmaskedContext = getUnmaskedContext(workInProgress, ctor, true); instance.props = newProps; instance.state = workInProgress.memoizedState; @@ -758,7 +758,11 @@ function resumeMountClassInstance( instance.props = oldProps; const oldContext = instance.context; - const nextLegacyUnmaskedContext = getUnmaskedContext(workInProgress, ctor); + const nextLegacyUnmaskedContext = getUnmaskedContext( + workInProgress, + ctor, + true, + ); const nextLegacyContext = getMaskedContext( workInProgress, nextLegacyUnmaskedContext, @@ -897,7 +901,11 @@ function updateClassInstance( instance.props = oldProps; const oldContext = instance.context; - const nextLegacyUnmaskedContext = getUnmaskedContext(workInProgress, ctor); + const nextLegacyUnmaskedContext = getUnmaskedContext( + workInProgress, + ctor, + true, + ); const nextLegacyContext = getMaskedContext( workInProgress, nextLegacyUnmaskedContext, diff --git a/packages/react-reconciler/src/ReactFiberContext.js b/packages/react-reconciler/src/ReactFiberContext.js index 0a12e724b16..e31f50ae549 100644 --- a/packages/react-reconciler/src/ReactFiberContext.js +++ b/packages/react-reconciler/src/ReactFiberContext.js @@ -49,11 +49,12 @@ let previousContext: Object = emptyContextObject; function getUnmaskedContext( workInProgress: Fiber, Component: Function, + didPushOwnContext: boolean, ): Object { const hasOwnContext = isContextProvider(Component); - if (hasOwnContext) { + if (hasOwnContext && didPushOwnContext) { // 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 + // we may 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; From 44b4c13d120fc17ab1904facdd0ca740243fca3a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 20 Aug 2018 14:43:32 +0100 Subject: [PATCH 3/4] Put cheaper check first --- packages/react-reconciler/src/ReactFiberContext.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberContext.js b/packages/react-reconciler/src/ReactFiberContext.js index e31f50ae549..65aa74aab97 100644 --- a/packages/react-reconciler/src/ReactFiberContext.js +++ b/packages/react-reconciler/src/ReactFiberContext.js @@ -51,8 +51,7 @@ function getUnmaskedContext( Component: Function, didPushOwnContext: boolean, ): Object { - const hasOwnContext = isContextProvider(Component); - if (hasOwnContext && didPushOwnContext) { + if (didPushOwnContext && isContextProvider(Component)) { // If the fiber is a context provider itself, when we read its context // we may have already pushed its own child context on the stack. A context // provider should not "see" its own child context. Therefore we read the From 580c56f92a957cece48cfd7a76eaf98d56ec96a2 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 20 Aug 2018 16:05:49 +0100 Subject: [PATCH 4/4] Naming --- packages/react-reconciler/src/ReactFiberContext.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberContext.js b/packages/react-reconciler/src/ReactFiberContext.js index 65aa74aab97..58c648ad762 100644 --- a/packages/react-reconciler/src/ReactFiberContext.js +++ b/packages/react-reconciler/src/ReactFiberContext.js @@ -49,9 +49,9 @@ let previousContext: Object = emptyContextObject; function getUnmaskedContext( workInProgress: Fiber, Component: Function, - didPushOwnContext: boolean, + didPushOwnContextIfProvider: boolean, ): Object { - if (didPushOwnContext && isContextProvider(Component)) { + if (didPushOwnContextIfProvider && isContextProvider(Component)) { // If the fiber is a context provider itself, when we read its context // we may have already pushed its own child context on the stack. A context // provider should not "see" its own child context. Therefore we read the