From f0924441c8522f8098c8c7ab32644c2077c26f73 Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Tue, 27 Mar 2018 21:29:06 -0400 Subject: [PATCH 1/8] Test case for React Context bailing out unexpectedly --- .../ReactNewContext-test.internal.js | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 4b3880fd626b3..0ec22d04fbbfd 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -762,6 +762,45 @@ describe('ReactNewContext', () => { expect(ReactNoop.getChildren()).toEqual([span('Child')]); }); + it('renders context children when child is an instance method', () => { + const Context = React.createContext(0); + + class App extends React.Component { + state = { + value: 0, + }; + + renderConsumer = (context) => { + ReactNoop.yield('App#renderConsumer'); + return ; + }; + + render() { + ReactNoop.yield('App'); + return ( + + {this.renderConsumer} + + ); + } + } + + // Initial mount + let inst; + ReactNoop.render( inst = ref} />); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); + expect(ReactNoop.getChildren()).toEqual([span(0)]); + + // Update + inst.setState({ value: 1}); + expect(ReactNoop.flush()).toEqual([ + 'App', + // Child does because it may read from the instance + 'App#renderConsumer', + ]); + expect(ReactNoop.getChildren()).toEqual([span(1)]); + }); + // This is a regression case for https://github.com/facebook/react/issues/12389. it('does not run into an infinite loop', () => { const Context = React.createContext(null); From 8686c0f6bdc1cba3056fb2212f3f7740c749d33a Mon Sep 17 00:00:00 2001 From: Dustan Kasten Date: Tue, 27 Mar 2018 22:00:35 -0400 Subject: [PATCH 2/8] =?UTF-8?q?This=20is=20=F0=9F=92=AF%=20definitely=20no?= =?UTF-8?q?t=20the=20correct=20fix=20at=20all?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../react-reconciler/src/ReactFiberBeginWork.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index f3f9681332dcf..dc85ae81eaacd 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -995,8 +995,18 @@ export default function( renderExpirationTime, ); } else if (oldProps !== null && oldProps.children === newProps.children) { - // No change. Bailout early if children are the same. - return bailoutOnAlreadyFinishedWork(current, workInProgress); + let bail = true; + const stateNode = current.return.return.stateNode; + for (const key in stateNode) { + if (stateNode[key] === newProps.children) { + bail = false; + break; + } + } + if (bail) { + // No change. Bailout early if children are the same. + return bailoutOnAlreadyFinishedWork(current, workInProgress); + } } const render = newProps.children; From f5c1ca0435ac618a22f9550fbfa158c73633b87b Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 29 Mar 2018 17:23:04 +0100 Subject: [PATCH 3/8] =?UTF-8?q?Revert=20"This=20is=20=F0=9F=92=AF%=20defin?= =?UTF-8?q?itely=20not=20the=20correct=20fix=20at=20all"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 8686c0f6bdc1cba3056fb2212f3f7740c749d33a. --- .../react-reconciler/src/ReactFiberBeginWork.js | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index dc85ae81eaacd..f3f9681332dcf 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -995,18 +995,8 @@ export default function( renderExpirationTime, ); } else if (oldProps !== null && oldProps.children === newProps.children) { - let bail = true; - const stateNode = current.return.return.stateNode; - for (const key in stateNode) { - if (stateNode[key] === newProps.children) { - bail = false; - break; - } - } - if (bail) { - // No change. Bailout early if children are the same. - return bailoutOnAlreadyFinishedWork(current, workInProgress); - } + // No change. Bailout early if children are the same. + return bailoutOnAlreadyFinishedWork(current, workInProgress); } const render = newProps.children; From 29922eb1e0492216dc6cd597373d1dd1640d3fab Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 29 Mar 2018 17:25:33 +0100 Subject: [PATCH 4/8] Formatting + minor tweaks to the test --- .../__tests__/ReactNewContext-test.internal.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 0ec22d04fbbfd..50d75333ee6ca 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -762,7 +762,10 @@ describe('ReactNewContext', () => { expect(ReactNoop.getChildren()).toEqual([span('Child')]); }); - it('renders context children when child is an instance method', () => { + // The closure may reference this.state, and if we did bail out, it would've been + // very confusing. Calling setState() would have no effect if the render callback + // is auto-bound. https://github.com/facebook/react/pull/12470#issuecomment-376917711 + it('does not bail out when context child closure is referentially equal', () => { const Context = React.createContext(0); class App extends React.Component { @@ -770,7 +773,7 @@ describe('ReactNewContext', () => { value: 0, }; - renderConsumer = (context) => { + renderConsumer = context => { ReactNoop.yield('App#renderConsumer'); return ; }; @@ -787,17 +790,13 @@ describe('ReactNewContext', () => { // Initial mount let inst; - ReactNoop.render( inst = ref} />); + ReactNoop.render( (inst = ref)} />); expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); expect(ReactNoop.getChildren()).toEqual([span(0)]); // Update - inst.setState({ value: 1}); - expect(ReactNoop.flush()).toEqual([ - 'App', - // Child does because it may read from the instance - 'App#renderConsumer', - ]); + inst.setState({value: 1}); + expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); expect(ReactNoop.getChildren()).toEqual([span(1)]); }); From f7ce2f95759e259a037349c78026d0ac629fb445 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 29 Mar 2018 18:14:29 +0100 Subject: [PATCH 5/8] Don't bail out on consumer child equality --- .../src/ReactFiberBeginWork.js | 6 +- .../ReactNewContext-test.internal.js | 90 ++++++++++++------- 2 files changed, 61 insertions(+), 35 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index f3f9681332dcf..fb96a14d9d5e0 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -994,10 +994,10 @@ export default function( changedBits, renderExpirationTime, ); - } else if (oldProps !== null && oldProps.children === newProps.children) { - // No change. Bailout early if children are the same. - return bailoutOnAlreadyFinishedWork(current, workInProgress); } + // There is no bailout on `children` equality because we expect people + // to often pass a bound method as a child, but it may reference + // `this.state` (and thus may need to re-render on `setState`). const render = newProps.children; diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index 50d75333ee6ca..e48eab66d2a0d 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -727,55 +727,81 @@ describe('ReactNewContext', () => { expect(ReactNoop.getChildren()).toEqual([span('Child')]); }); - it('consumer bails out if children and value are unchanged (like sCU)', () => { + it('consumer bails out if value is unchanged and something above bailed out', () => { const Context = React.createContext(0); - function Child() { - ReactNoop.yield('Child'); - return ; + function renderChildValue(value) { + ReactNoop.yield('Consumer'); + return } - function renderConsumer(context) { - return ; + function ChildWithInlineRenderCallback() { + ReactNoop.yield('ChildWithInlineRenderCallback'); + // Note: we are intentionally passing an inline arrow. Don't refactor. + return {value => renderChildValue(value)}; } - function App(props) { - ReactNoop.yield('App'); - return ( - - {renderConsumer} - - ); + function ChildWithCachedRenderCallback() { + ReactNoop.yield('ChildWithCachedRenderCallback'); + return {renderChildValue}; + } + + class PureIndirection extends React.PureComponent { + render() { + ReactNoop.yield('PureIndirection'); + return ( + + + + + ); + } + } + + class App extends React.Component { + render() { + ReactNoop.yield('App'); + return ( + + + + ); + } } // Initial mount - ReactNoop.render(); - expect(ReactNoop.flush()).toEqual(['App', 'Child']); - expect(ReactNoop.getChildren()).toEqual([span('Child')]); + let inst; + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App', 'PureIndirection', 'ChildWithInlineRenderCallback', 'Consumer', 'ChildWithCachedRenderCallback', 'Consumer']); + expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]); - // Update - ReactNoop.render(); - expect(ReactNoop.flush()).toEqual([ - 'App', - // Child does not re-render - ]); - expect(ReactNoop.getChildren()).toEqual([span('Child')]); + // Update (bailout) + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App']); + expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]); + + // Update (no bailout) + ReactNoop.render( (inst = ref)} />); + expect(ReactNoop.flush()).toEqual(['App', 'Consumer', 'Consumer']); + expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]); }); - // The closure may reference this.state, and if we did bail out, it would've been - // very confusing. Calling setState() would have no effect if the render callback - // is auto-bound. https://github.com/facebook/react/pull/12470#issuecomment-376917711 - it('does not bail out when context child closure is referentially equal', () => { + // Context consumer bails out on propagating "deep" updates when `value` hasn't changed. + // However, it doesn't bail out from rendering if the component above it re-rendered anyway. + // If we bailed out on referential equality, it would be confusing that you + // can call this.setState(), but an autobound render callback "blocked" the update. + // https://github.com/facebook/react/pull/12470#issuecomment-376917711 + it('consumer does not bail out if there were no bailouts above it', () => { const Context = React.createContext(0); class App extends React.Component { state = { - value: 0, + text: 'hello', }; renderConsumer = context => { ReactNoop.yield('App#renderConsumer'); - return ; + return ; }; render() { @@ -792,12 +818,12 @@ describe('ReactNewContext', () => { let inst; ReactNoop.render( (inst = ref)} />); expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); - expect(ReactNoop.getChildren()).toEqual([span(0)]); + expect(ReactNoop.getChildren()).toEqual([span('hello')]); // Update - inst.setState({value: 1}); + inst.setState({text: 'goodbye'}); expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']); - expect(ReactNoop.getChildren()).toEqual([span(1)]); + expect(ReactNoop.getChildren()).toEqual([span('goodbye')]); }); // This is a regression case for https://github.com/facebook/react/issues/12389. From 37750b7afd5c4d264e661670a4478b1e34d28371 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 29 Mar 2018 18:18:05 +0100 Subject: [PATCH 6/8] Tweak the comment --- packages/react-reconciler/src/ReactFiberBeginWork.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index fb96a14d9d5e0..7ca1aa6f25574 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -997,7 +997,7 @@ export default function( } // There is no bailout on `children` equality because we expect people // to often pass a bound method as a child, but it may reference - // `this.state` (and thus may need to re-render on `setState`). + // `this.state` or `this.props` (and thus needs to re-render on `setState`). const render = newProps.children; From 140162b2a8b07c9e8a8bd2364960f979cfe297c9 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 29 Mar 2018 18:56:45 +0100 Subject: [PATCH 7/8] Pretty lint --- .../__tests__/ReactNewContext-test.internal.js | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index e48eab66d2a0d..b6566e9591bd5 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -732,13 +732,15 @@ describe('ReactNewContext', () => { function renderChildValue(value) { ReactNoop.yield('Consumer'); - return + return ; } function ChildWithInlineRenderCallback() { ReactNoop.yield('ChildWithInlineRenderCallback'); // Note: we are intentionally passing an inline arrow. Don't refactor. - return {value => renderChildValue(value)}; + return ( + {value => renderChildValue(value)} + ); } function ChildWithCachedRenderCallback() { @@ -770,9 +772,15 @@ describe('ReactNewContext', () => { } // Initial mount - let inst; ReactNoop.render( (inst = ref)} />); - expect(ReactNoop.flush()).toEqual(['App', 'PureIndirection', 'ChildWithInlineRenderCallback', 'Consumer', 'ChildWithCachedRenderCallback', 'Consumer']); + expect(ReactNoop.flush()).toEqual([ + 'App', + 'PureIndirection', + 'ChildWithInlineRenderCallback', + 'Consumer', + 'ChildWithCachedRenderCallback', + 'Consumer', + ]); expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]); // Update (bailout) From b4aa7c95103a65c8297f4ceb730692165900da4a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 29 Mar 2018 19:06:23 +0100 Subject: [PATCH 8/8] Silly Dan --- .../src/__tests__/ReactNewContext-test.internal.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js index b6566e9591bd5..c0aadacd1c030 100644 --- a/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js @@ -772,7 +772,7 @@ describe('ReactNewContext', () => { } // Initial mount - ReactNoop.render( (inst = ref)} />); + ReactNoop.render(); expect(ReactNoop.flush()).toEqual([ 'App', 'PureIndirection', @@ -784,12 +784,12 @@ describe('ReactNewContext', () => { expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]); // Update (bailout) - ReactNoop.render( (inst = ref)} />); + ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['App']); expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]); // Update (no bailout) - ReactNoop.render( (inst = ref)} />); + ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['App', 'Consumer', 'Consumer']); expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]); });