[Fiber] Some setState related issues (#8010)
* Use the memoized props/state from the workInProgress We don't want to use the "current" props/state because if we have started work, then pause and continue then we'll have the newer props/state on it already. If it is not a resume, it should be the same as current. * Deprioritize setState within a deprioritized tree Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority. This ensures that we don't let a deprioritized subtree get reconciled at a higher priority. * Bump idx in unit test This proves that this number is actually deprioritized.
Sebastian Markbåge committed
Oct 19, 2016 at 13:32 UTC
a5195102d4278cd4e2c36612afa7f1fc09b201fd
3 files changed
+234
-6
src/renderers/shared/fiber/ReactFiberBeginWork.js
+35
-6
@@ -160,15 +160,24 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, s
160
// Account for the possibly of missing pending props by falling back to the
161
// memoized props.
162
var props = workInProgress.pendingProps;
163
- if (!props && current) {
164
- props = current.memoizedProps;
163
+ if (!props) {
164
+ // If there isn't any new props, then we'll reuse the memoized props.
165
+ // This could be from already completed work.
166
+ props = workInProgress.memoizedProps;
167
+ if (!props) {
168
+ throw new Error('There should always be pending or memoized props.');
169
+ }
170
}
171
+
172
// Compute the state using the memoized state and the update queue.
173
var updateQueue = workInProgress.updateQueue;
168
- var previousState = current ? current.memoizedState : null;
169
- var state = updateQueue ?
170
- mergeUpdateQueue(updateQueue, previousState, props) :
171
- previousState;
174
+ var previousState = workInProgress.memoizedState;
175
+ var state;
176
+ if (updateQueue) {
177
+ state = mergeUpdateQueue(updateQueue, previousState, props);
178
+ } else {
179
+ state = previousState;
180
+ }
181
182
var instance = workInProgress.stateNode;
183
if (!instance) {
@@ -300,6 +309,26 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, s
309
function bailoutOnAlreadyFinishedWork(current, workInProgress : Fiber) : ?Fiber {
310
const priorityLevel = workInProgress.pendingWorkPriority;
311
312
+ if (workInProgress.tag === HostComponent &&
313
+ workInProgress.memoizedProps.hidden &&
314
+ workInProgress.pendingWorkPriority !== OffscreenPriority) {
315
+ // This subtree still has work, but it should be deprioritized so we need
316
+ // to bail out and not do any work yet.
317
+ // TODO: It would be better if this tree got its correct priority set
318
+ // during scheduleUpdate instead because otherwise we'll start a higher
319
+ // priority reconciliation first before we can get down here. However,
320
+ // that is a bit tricky since workInProgress and current can have
321
+ // different "hidden" settings.
322
+ let child = workInProgress.progressedChild;
323
+ while (child) {
324
+ // To ensure that this subtree gets its priority reset, the children
325
+ // need to be reset.
326
+ child.pendingWorkPriority = OffscreenPriority;
327
+ child = child.sibling;
328
+ }
329
+ return null;
330
+ }
331
+
332
// TODO: We should ideally be able to bail out early if the children have no
333
// more work to do. However, since we don't have a separation of this
334
// Fiber's priority and its children yet - we don't know without doing lots
src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
+92
@@ -835,4 +835,96 @@ describe('ReactIncremental', () => {
835
ReactNoop.flush();
836
expect(ops).toEqual(['Foo', 'Bar', 'Baz', 'Bar', 'Baz']);
837
});
838
+
839
+ it('can call sCU while resuming a partly mounted component', () => {
840
+ var ops = [];
841
+
842
+ class Bar extends React.Component {
843
+ state = { y: 'A' };
844
+ shouldComponentUpdate(newProps, newState) {
845
+ return this.props.x !== newProps.x ||
846
+ this.state.y !== newState.y;
847
+ }
848
+ render() {
849
+ ops.push('Bar:' + this.props.x);
850
+ return <span prop={'' + (this.props.x === this.state.y)} />;
851
+ }
852
+ }
853
+
854
+ function Foo(props) {
855
+ ops.push('Foo');
856
+ return [
857
+ <Bar key="a" x="A" />,
858
+ <Bar key="b" x="B" />,
859
+ <Bar key="c" x="C" />,
860
+ ];
861
+ }
862
+
863
+ ReactNoop.render(<Foo />);
864
+ ReactNoop.flushDeferredPri(30);
865
+ expect(ops).toEqual(['Foo', 'Bar:A', 'Bar:B']);
866
+
867
+ ops = [];
868
+
869
+ ReactNoop.render(<Foo />);
870
+ ReactNoop.flushDeferredPri(40);
871
+ expect(ops).toEqual(['Foo', 'Bar:B', 'Bar:C']);
872
+ });
873
+
874
+ it('gets new props when setting state on a partly updated component', () => {
875
+ var ops = [];
876
+ var instances = [];
877
+
878
+ class Bar extends React.Component {
879
+ state = { y: 'A' };
880
+ constructor() {
881
+ super();
882
+ instances.push(this);
883
+ }
884
+ performAction() {
885
+ this.setState({
886
+ y: 'B',
887
+ });
888
+ }
889
+ render() {
890
+ ops.push('Bar:' + this.props.x + '-' + this.props.step);
891
+ return <span prop={'' + (this.props.x === this.state.y)} />;
892
+ }
893
+ }
894
+
895
+ function Baz() {
896
+ // This component is used as a sibling to Foo so that we can fully
897
+ // complete Foo, without committing.
898
+ ops.push('Baz');
899
+ return <div />;
900
+ }
901
+
902
+ function Foo(props) {
903
+ ops.push('Foo');
904
+ return [
905
+ <Bar key="a" x="A" step={props.step} />,
906
+ <Bar key="b" x="B" step={props.step} />,
907
+ ];
908
+ }
909
+
910
+ ReactNoop.render(<div><Foo step={0} /><Baz /><Baz /></div>);
911
+ ReactNoop.flush();
912
+
913
+ ops = [];
914
+
915
+ // Flush part way through with new props, fully completing the first Bar.
916
+ // However, it doesn't commit yet.
917
+ ReactNoop.render(<div><Foo step={1} /><Baz /><Baz /></div>);
918
+ ReactNoop.flushDeferredPri(45);
919
+ expect(ops).toEqual(['Foo', 'Bar:A-1', 'Bar:B-1', 'Baz']);
920
+
921
+ // Make an update to the same Bar.
922
+ instances[0].performAction();
923
+
924
+ ops = [];
925
+
926
+ ReactNoop.flush();
927
+ expect(ops).toEqual(['Bar:A-1', 'Baz', 'Baz']);
928
+ });
929
+
930
});
src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js
+107
@@ -601,6 +601,113 @@ describe('ReactIncrementalSideEffects', () => {
601
expect(ops).toEqual(['Baz', 'Bar', 'Baz', 'Bar', 'Bar']);
602
});
603
604
+ it('deprioritizes setStates that happens within a deprioritized tree', () => {
605
+ var ops = [];
606
+
607
+ var barInstances = [];
608
+
609
+ class Bar extends React.Component {
610
+ constructor() {
611
+ super();
612
+ this.state = { active: false };
613
+ barInstances.push(this);
614
+ }
615
+ activate() {
616
+ this.setState({ active: true });
617
+ }
618
+ render() {
619
+ ops.push('Bar');
620
+ return <span prop={this.state.active ? 'X' : this.props.idx} />;
621
+ }
622
+ }
623
+ function Foo(props) {
624
+ ops.push('Foo');
625
+ return (
626
+ <div>
627
+ <span prop={props.tick} />
628
+ <div hidden={true}>
629
+ <Bar idx={props.idx} />
630
+ <Bar idx={props.idx} />
631
+ <Bar idx={props.idx} />
632
+ </div>
633
+ </div>
634
+ );
635
+ }
636
+ ReactNoop.render(<Foo tick={0} idx={0} />);
637
+ ReactNoop.flush();
638
+ expect(ReactNoop.root.children).toEqual([
639
+ div(
640
+ span(0),
641
+ div(
642
+ span(0),
643
+ span(0),
644
+ span(0)
645
+ )
646
+ ),
647
+ ]);
648
+
649
+ expect(ops).toEqual(['Foo', 'Bar', 'Bar', 'Bar']);
650
+
651
+ ops = [];
652
+
653
+ ReactNoop.render(<Foo tick={1} idx={1} />);
654
+ ReactNoop.flushDeferredPri(70);
655
+ expect(ReactNoop.root.children).toEqual([
656
+ div(
657
+ // Updated.
658
+ span(1),
659
+ div(
660
+ // Still not updated.
661
+ span(0),
662
+ span(0),
663
+ span(0)
664
+ )
665
+ ),
666
+ ]);
667
+
668
+ expect(ops).toEqual(['Foo', 'Bar', 'Bar']);
669
+ ops = [];
670
+
671
+ barInstances[0].activate();
672
+
673
+ // This should not be enough time to render the content of all the hidden
674
+ // items. Including the set state since that is deprioritized.
675
+ // TODO: The cycles it takes to do this could be lowered with further
676
+ // optimizations.
677
+ ReactNoop.flushDeferredPri(60);
678
+ expect(ReactNoop.root.children).toEqual([
679
+ div(
680
+ // Updated.
681
+ span(1),
682
+ div(
683
+ // Still not updated.
684
+ span(0),
685
+ span(0),
686
+ span(0)
687
+ )
688
+ ),
689
+ ]);
690
+
691
+ expect(ops).toEqual(['Bar']);
692
+ ops = [];
693
+
694
+ // However, once we render fully, we will have enough time to finish it all
695
+ // at once.
696
+ ReactNoop.flush();
697
+ expect(ReactNoop.root.children).toEqual([
698
+ div(
699
+ span(1),
700
+ div(
701
+ // Now we had enough time to finish the spans.
702
+ span('X'),
703
+ span(1),
704
+ span(1),
705
+ )
706
+ ),
707
+ ]);
708
+
709
+ expect(ops).toEqual(['Bar', 'Bar']);
710
+ });
711
// TODO: Test that side-effects are not cut off when a work in progress node
712
// moves to "current" without flushing due to having lower priority. Does this
713
// even happen? Maybe a child doesn't get processed because it is lower prio?