Don't bail out on referential equality of Consumer's props.children function (#12470)
* Test case for React Context bailing out unexpectedly * This is 💯% definitely not the correct fix at all * Revert "This is 💯% definitely not the correct fix at all" This reverts commit 8686c0f6bdc1cba3056fb2212f3f7740c749d33a. * Formatting + minor tweaks to the test * Don't bail out on consumer child equality * Tweak the comment * Pretty lint * Silly Dan
Dustan Kasten committed
Mar 29, 2018 at 14:16 UTC
15e3dffb4c9ca9b9466f4ef1a6b8b2293d41e9d6
2 files changed
+95
-23
packages/react-reconciler/src/ReactFiberBeginWork.js
+3
-3
@@ -994,10 +994,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
994
changedBits,
995
renderExpirationTime,
996
);
997
- } else if (oldProps !== null && oldProps.children === newProps.children) {
998
- // No change. Bailout early if children are the same.
999
- return bailoutOnAlreadyFinishedWork(current, workInProgress);
997
}
998
+ // There is no bailout on `children` equality because we expect people
999
+ // to often pass a bound method as a child, but it may reference
1000
+ // `this.state` or `this.props` (and thus needs to re-render on `setState`).
1001
1002
const render = newProps.children;
1003
packages/react-reconciler/src/__tests__/ReactNewContext-test.internal.js
+92
-20
@@ -727,39 +727,111 @@ describe('ReactNewContext', () => {
727
expect(ReactNoop.getChildren()).toEqual([span('Child')]);
728
});
729
730
- it('consumer bails out if children and value are unchanged (like sCU)', () => {
730
+ it('consumer bails out if value is unchanged and something above bailed out', () => {
731
const Context = React.createContext(0);
732
733
- function Child() {
734
- ReactNoop.yield('Child');
735
- return <span prop="Child" />;
736
- }
737
-
738
- function renderConsumer(context) {
739
- return <Child context={context} />;
733
+ function renderChildValue(value) {
734
+ ReactNoop.yield('Consumer');
735
+ return <span prop={value} />;
736
}
737
742
- function App(props) {
743
- ReactNoop.yield('App');
738
+ function ChildWithInlineRenderCallback() {
739
+ ReactNoop.yield('ChildWithInlineRenderCallback');
740
+ // Note: we are intentionally passing an inline arrow. Don't refactor.
741
return (
745
- <Context.Provider value={props.value}>
746
- <Context.Consumer>{renderConsumer}</Context.Consumer>
747
- </Context.Provider>
742
+ <Context.Consumer>{value => renderChildValue(value)}</Context.Consumer>
743
);
744
}
745
751
- // Initial mount
752
- ReactNoop.render(<App value={1} />);
753
- expect(ReactNoop.flush()).toEqual(['App', 'Child']);
754
- expect(ReactNoop.getChildren()).toEqual([span('Child')]);
746
+ function ChildWithCachedRenderCallback() {
747
+ ReactNoop.yield('ChildWithCachedRenderCallback');
748
+ return <Context.Consumer>{renderChildValue}</Context.Consumer>;
749
+ }
750
756
- // Update
751
+ class PureIndirection extends React.PureComponent {
752
+ render() {
753
+ ReactNoop.yield('PureIndirection');
754
+ return (
755
+ <React.Fragment>
756
+ <ChildWithInlineRenderCallback />
757
+ <ChildWithCachedRenderCallback />
758
+ </React.Fragment>
759
+ );
760
+ }
761
+ }
762
+
763
+ class App extends React.Component {
764
+ render() {
765
+ ReactNoop.yield('App');
766
+ return (
767
+ <Context.Provider value={this.props.value}>
768
+ <PureIndirection />
769
+ </Context.Provider>
770
+ );
771
+ }
772
+ }
773
+
774
+ // Initial mount
775
ReactNoop.render(<App value={1} />);
776
expect(ReactNoop.flush()).toEqual([
777
'App',
760
- // Child does not re-render
778
+ 'PureIndirection',
779
+ 'ChildWithInlineRenderCallback',
780
+ 'Consumer',
781
+ 'ChildWithCachedRenderCallback',
782
+ 'Consumer',
783
]);
762
- expect(ReactNoop.getChildren()).toEqual([span('Child')]);
784
+ expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]);
785
+
786
+ // Update (bailout)
787
+ ReactNoop.render(<App value={1} />);
788
+ expect(ReactNoop.flush()).toEqual(['App']);
789
+ expect(ReactNoop.getChildren()).toEqual([span(1), span(1)]);
790
+
791
+ // Update (no bailout)
792
+ ReactNoop.render(<App value={2} />);
793
+ expect(ReactNoop.flush()).toEqual(['App', 'Consumer', 'Consumer']);
794
+ expect(ReactNoop.getChildren()).toEqual([span(2), span(2)]);
795
+ });
796
+
797
+ // Context consumer bails out on propagating "deep" updates when `value` hasn't changed.
798
+ // However, it doesn't bail out from rendering if the component above it re-rendered anyway.
799
+ // If we bailed out on referential equality, it would be confusing that you
800
+ // can call this.setState(), but an autobound render callback "blocked" the update.
801
+ // https://github.com/facebook/react/pull/12470#issuecomment-376917711
802
+ it('consumer does not bail out if there were no bailouts above it', () => {
803
+ const Context = React.createContext(0);
804
+
805
+ class App extends React.Component {
806
+ state = {
807
+ text: 'hello',
808
+ };
809
+
810
+ renderConsumer = context => {
811
+ ReactNoop.yield('App#renderConsumer');
812
+ return <span prop={this.state.text} />;
813
+ };
814
+
815
+ render() {
816
+ ReactNoop.yield('App');
817
+ return (
818
+ <Context.Provider value={this.props.value}>
819
+ <Context.Consumer>{this.renderConsumer}</Context.Consumer>
820
+ </Context.Provider>
821
+ );
822
+ }
823
+ }
824
+
825
+ // Initial mount
826
+ let inst;
827
+ ReactNoop.render(<App value={1} ref={ref => (inst = ref)} />);
828
+ expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']);
829
+ expect(ReactNoop.getChildren()).toEqual([span('hello')]);
830
+
831
+ // Update
832
+ inst.setState({text: 'goodbye'});
833
+ expect(ReactNoop.flush()).toEqual(['App', 'App#renderConsumer']);
834
+ expect(ReactNoop.getChildren()).toEqual([span('goodbye')]);
835
});
836
837
// This is a regression case for https://github.com/facebook/react/issues/12389.