Call refs and lifecycles on indeterminate components (#8614)
This was a bug because of the split between ClassComponent and IndeterminateComponent -- we didn't mark effects or push context when we come from an indeterminate component. Now they behave the same way. The code is even clumsier than before but I'm pretty worried we'll screw these up in the future if we don't unify the paths. Not many components exercise this path but Relay containers do so it's noticeable.
Ben Alpert committed
Dec 21, 2016 at 13:51 UTC
2a5fe4c2b021ad9a67b904aceed6bb10fe160a79
3 files changed
+58
-4
scripts/fiber/tests-passing.txt
+1
@@ -1343,6 +1343,7 @@ src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js
1343
* should not throw when updating an auxiliary component
1344
* should allow state updates in componentDidMount
1345
* should call nested lifecycle methods in the right order
1346
+* calls effects on module-pattern component
1347
1348
src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
1349
* should support module pattern components
src/renderers/shared/fiber/ReactFiberBeginWork.js
+6
-4
@@ -229,7 +229,10 @@ module.exports = function<T, P, I, TI, C, CX>(
229
} else {
230
shouldUpdate = updateClassInstance(current, workInProgress, priorityLevel);
231
}
232
+ return finishClassComponent(current, workInProgress, shouldUpdate);
233
+ }
234
235
+ function finishClassComponent(current : ?Fiber, workInProgress : Fiber, shouldUpdate : boolean) {
236
// Schedule side-effects
237
if (shouldUpdate) {
238
workInProgress.effectTag |= Update;
@@ -411,14 +414,13 @@ module.exports = function<T, P, I, TI, C, CX>(
414
workInProgress.tag = ClassComponent;
415
adoptClassInstance(workInProgress, value);
416
mountClassInstance(workInProgress, priorityLevel);
414
- ReactCurrentOwner.current = workInProgress;
415
- value = value.render();
417
+ return finishClassComponent(current, workInProgress, true);
418
} else {
419
// Proceed under the assumption that this is a functional component
420
workInProgress.tag = FunctionalComponent;
421
+ reconcileChildren(current, workInProgress, value);
422
+ return workInProgress.child;
423
}
420
- reconcileChildren(current, workInProgress, value);
421
- return workInProgress.child;
424
}
425
426
function updateCoroutineComponent(current, workInProgress) {
src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js
+51
@@ -594,4 +594,55 @@ describe('ReactComponentLifeCycle', () => {
594
'inner componentWillUnmount',
595
]);
596
});
597
+
598
+ it('calls effects on module-pattern component', function() {
599
+ const log = [];
600
+
601
+ function Parent() {
602
+ return {
603
+ render() {
604
+ expect(typeof this.props).toBe('object');
605
+ log.push('render');
606
+ return <Child />;
607
+ },
608
+ componentWillMount() {
609
+ log.push('will mount');
610
+ },
611
+ componentDidMount() {
612
+ log.push('did mount');
613
+ },
614
+ componentDidUpdate() {
615
+ log.push('did update');
616
+ },
617
+ getChildContext() {
618
+ return {x: 2};
619
+ },
620
+ };
621
+ }
622
+ Parent.childContextTypes = {
623
+ x: React.PropTypes.number,
624
+ };
625
+ function Child(props, context) {
626
+ expect(context.x).toBe(2);
627
+ return <div />;
628
+ }
629
+ Child.contextTypes = {
630
+ x: React.PropTypes.number,
631
+ };
632
+
633
+ const div = document.createElement('div');
634
+ ReactDOM.render(<Parent ref={(c) => c && log.push('ref')} />, div);
635
+ ReactDOM.render(<Parent ref={(c) => c && log.push('ref')} />, div);
636
+
637
+ expect(log).toEqual([
638
+ 'will mount',
639
+ 'render',
640
+ 'did mount',
641
+ 'ref',
642
+
643
+ 'render',
644
+ 'did update',
645
+ 'ref',
646
+ ]);
647
+ });
648
});