@samitouri / QOS-React-2 / commits / 9c25538e13

Fix resuming bug

If we abort work but have some completed, we can bail out if the shouldComponentUpdate returns true. However, then we have a tree that is low priority. When we bailout we currently use the "current" tree in this case. I don't think this is right. I'm not sure why I did that. Similarly, when we complete we use the "current" props if we didn't have pending props to do. However, we should be using the memoized props of that tree if it is a pending work tree. Added a unit test that covers these two cases.

Sebastian Markbage committed Oct 10, 2016 at 15:33 UTC 9c25538e13bcd3cc473b69f6cbaec64e77ddd196

3 files changed +150 -15

src/renderers/shared/fiber/ReactFiberBeginWork.js

+2 -11

@@ -402,17 +402,8 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>, g

402 }

403

404 function bailoutOnLowPriority(current, workInProgress) {

405 - if (current) {

406 - // TODO: If I have started work on this node, mark it as finished, then

407 - // return do other work, come back and hit this node... we killed that

408 - // work. It is now in an inconsistent state. We probably need to check

409 - // progressedChild or something.

410 - workInProgress.child = current.child;

411 - workInProgress.memoizedProps = current.memoizedProps;

412 - workInProgress.output = current.output;

413 - workInProgress.firstEffect = null;

414 - workInProgress.lastEffect = null;

415 - }

405 + // TODO: What if this is currently in progress?

406 + // How can that happen? How is this not being cloned?

407 return null;

408 }

409

src/renderers/shared/fiber/ReactFiberCompleteWork.js

+1 -1

@@ -151,7 +151,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {

151 // TODO: Split the update API as separate for the props vs. children.

152 // Even better would be if children weren't special cased at all tho.

153 if (!newProps) {

154 - newProps = oldProps;

154 + newProps = workInProgress.memoizedProps || oldProps;

155 }

156 const instance : I = workInProgress.stateNode;

157 if (prepareUpdate(instance, oldProps, newProps)) {

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

+147 -3

@@ -380,7 +380,7 @@ describe('ReactIncrementalSideEffects', () => {

380 it('can defer side-effects and resume them later on', function() {

381 class Bar extends React.Component {

382 shouldComponentUpdate(nextProps) {

383 - return this.props.idx !== nextProps;

383 + return this.props.idx !== nextProps.idx;

384 }

385 render() {

386 return <span prop={this.props.idx} />;

@@ -437,10 +437,11 @@ describe('ReactIncrementalSideEffects', () => {

437 )

438 ),

439 ]);

440 - ReactNoop.flushDeferredPri(30);

440 + ReactNoop.render(<Foo tick={3} idx={1} />);

441 + ReactNoop.flush();

442 expect(ReactNoop.root.children).toEqual([

443 div(

443 - span(2),

444 + span(3),

445 div(

446 // New numbers.

447 span(1),

@@ -456,6 +457,149 @@ describe('ReactIncrementalSideEffects', () => {

457 expect(innerSpanA).toBe(innerSpanB);

458 });

459

460 + it('can defer side-effects and reuse them later - complex', function() {

461 + var ops = [];

462 +

463 + class Bar extends React.Component {

464 + shouldComponentUpdate(nextProps) {

465 + return this.props.idx !== nextProps.idx;

466 + }

467 + render() {

468 + ops.push('Bar');

469 + return <span prop={this.props.idx} />;

470 + }

471 + }

472 + class Baz extends React.Component {

473 + shouldComponentUpdate(nextProps) {

474 + return this.props.idx !== nextProps.idx;

475 + }

476 + render() {

477 + ops.push('Baz');

478 + return [<Bar idx={this.props.idx} />, <Bar idx={this.props.idx} />];

479 + }

480 + }

481 + function Foo(props) {

482 + ops.push('Foo');

483 + return (

484 + <div>

485 + <span prop={props.tick} />

486 + <div hidden={true}>

487 + <Baz idx={props.idx} />

488 + <Baz idx={props.idx} />

489 + <Baz idx={props.idx} />

490 + </div>

491 + </div>

492 + );

493 + }

494 + ReactNoop.render(<Foo tick={0} idx={0} />);

495 + ReactNoop.flushDeferredPri(65);

496 + expect(ReactNoop.root.children).toEqual([

497 + div(

498 + span(0),

499 + div(/*the spans are down-prioritized and not rendered yet*/)

500 + ),

501 + ]);

502 +

503 + expect(ops).toEqual(['Foo', 'Baz', 'Bar']);

504 + ops = [];

505 +

506 + ReactNoop.render(<Foo tick={1} idx={0} />);

507 + ReactNoop.flushDeferredPri(70);

508 + expect(ReactNoop.root.children).toEqual([

509 + div(

510 + span(1),

511 + div(/*still not rendered yet*/)

512 + ),

513 + ]);

514 +

515 + expect(ops).toEqual(['Foo', 'Baz', 'Bar']);

516 + ops = [];

517 +

518 + ReactNoop.flush();

519 + expect(ReactNoop.root.children).toEqual([

520 + div(

521 + span(1),

522 + div(

523 + // Now we had enough time to finish the spans.

524 + span(0),

525 + span(0),

526 + span(0),

527 + span(0),

528 + span(0),

529 + span(0)

530 + )

531 + ),

532 + ]);

533 +

534 + expect(ops).toEqual(['Bar', 'Baz', 'Bar', 'Bar', 'Baz', 'Bar', 'Bar']);

535 + ops = [];

536 +

537 + // Now we're going to update the index but we'll only let it finish half

538 + // way through.

539 + ReactNoop.render(<Foo tick={2} idx={1} />);

540 + ReactNoop.flushDeferredPri(95);

541 + expect(ReactNoop.root.children).toEqual([

542 + div(

543 + span(2),

544 + div(

545 + // Still same old numbers.

546 + span(0),

547 + span(0),

548 + span(0),

549 + span(0),

550 + span(0),

551 + span(0)

552 + )

553 + ),

554 + ]);

555 +

556 + // We let it finish half way through. That means we'll have one fully

557 + // completed Baz, one half-way completed Baz and one fully incomplete Baz.

558 + expect(ops).toEqual(['Foo', 'Baz', 'Bar', 'Bar', 'Baz', 'Bar']);

559 + ops = [];

560 +

561 + // We'll update again, without letting the new index update yet. Only half

562 + // way through.

563 + ReactNoop.render(<Foo tick={3} idx={1} />);

564 + ReactNoop.flushDeferredPri(50);

565 + expect(ReactNoop.root.children).toEqual([

566 + div(

567 + span(3),

568 + div(

569 + // Old numbers.

570 + span(0),

571 + span(0),

572 + span(0),

573 + span(0),

574 + span(0),

575 + span(0)

576 + )

577 + ),

578 + ]);

579 +

580 + expect(ops).toEqual(['Foo']);

581 + ops = [];

582 +

583 + // We should now be able to reuse some of the work we've already done

584 + // and replay those side-effects.

585 + ReactNoop.flush();

586 + expect(ReactNoop.root.children).toEqual([

587 + div(

588 + span(3),

589 + div(

590 + // New numbers.

591 + span(1),

592 + span(1),

593 + span(1),

594 + span(1),

595 + span(1),

596 + span(1)

597 + )

598 + ),

599 + ]);

600 +

601 + expect(ops).toEqual(['Baz', 'Bar', 'Baz', 'Bar', 'Bar']);

602 + });

603

604 // TODO: Test that side-effects are not cut off when a work in progress node

605 // moves to "current" without flushing due to having lower priority. Does this