Skip to content

Commit f9f28f3

Browse files
gaearonjetoneza
authored andcommitted
Fix lazy() with defaultProps (facebook#14112)
* Resolve defaultProps for Lazy components * Make test fail again * Undo the partial fix * Make test output more compact * Add a separate failing test for sync mode * Clean up tests * Add another update to both tests * Resolve props for commit phase lifecycles * Resolve prevProps for begin phase lifecycles * Resolve prevProps for pre-commit lifecycles * Only resolve props if element type differs * Fix Flow * Don't set instance.props/state during commit phase This is an optimization. I'm not sure it's entirely safe. It's probably worth running internal tests and see if we can ever trigger a case where they're different. This can mess with resuming. * Keep setting instance.props/state before unmounting This reverts part of the previous commit. It broke a test that verifies we use current props in componentWillUnmount if the fiber unmounts due to an error.
1 parent 0409690 commit f9f28f3

5 files changed

Lines changed: 223 additions & 27 deletions

File tree

packages/react-reconciler/src/ReactFiberBeginWork.js

Lines changed: 4 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,10 @@ import {
103103
resumeMountClassInstance,
104104
updateClassInstance,
105105
} from './ReactFiberClassComponent';
106-
import {readLazyComponentType} from './ReactFiberLazyComponent';
106+
import {
107+
readLazyComponentType,
108+
resolveDefaultProps,
109+
} from './ReactFiberLazyComponent';
107110
import {
108111
resolveLazyComponentTag,
109112
createFiberFromTypeAndProps,
@@ -729,21 +732,6 @@ function updateHostText(current, workInProgress) {
729732
return null;
730733
}
731734

732-
function resolveDefaultProps(Component, baseProps) {
733-
if (Component && Component.defaultProps) {
734-
// Resolve default props. Taken from ReactElement
735-
const props = Object.assign({}, baseProps);
736-
const defaultProps = Component.defaultProps;
737-
for (let propName in defaultProps) {
738-
if (props[propName] === undefined) {
739-
props[propName] = defaultProps[propName];
740-
}
741-
}
742-
return props;
743-
}
744-
return baseProps;
745-
}
746-
747735
function mountLazyComponent(
748736
_current,
749737
workInProgress,

packages/react-reconciler/src/ReactFiberClassComponent.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import warningWithoutStack from 'shared/warningWithoutStack';
2828
import {REACT_CONTEXT_TYPE} from 'shared/ReactSymbols';
2929

3030
import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf';
31+
import {resolveDefaultProps} from './ReactFiberLazyComponent';
3132
import {StrictMode} from './ReactTypeOfMode';
3233

3334
import {
@@ -987,7 +988,10 @@ function updateClassInstance(
987988
const instance = workInProgress.stateNode;
988989

989990
const oldProps = workInProgress.memoizedProps;
990-
instance.props = oldProps;
991+
instance.props =
992+
workInProgress.type === workInProgress.elementType
993+
? oldProps
994+
: resolveDefaultProps(workInProgress.type, oldProps);
991995

992996
const oldContext = instance.context;
993997
const contextType = ctor.contextType;

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ import {onCommitUnmount} from './ReactFiberDevToolsHook';
6060
import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf';
6161
import {getStackByFiberInDevAndProd} from './ReactCurrentFiber';
6262
import {logCapturedError} from './ReactFiberErrorLogger';
63+
import {resolveDefaultProps} from './ReactFiberLazyComponent';
6364
import {getCommitTime} from './ReactProfilerTimer';
6465
import {commitUpdateQueue} from './ReactUpdateQueue';
6566
import {
@@ -228,10 +229,13 @@ function commitBeforeMutationLifeCycles(
228229
const prevState = current.memoizedState;
229230
startPhaseTimer(finishedWork, 'getSnapshotBeforeUpdate');
230231
const instance = finishedWork.stateNode;
231-
instance.props = finishedWork.memoizedProps;
232-
instance.state = finishedWork.memoizedState;
232+
// We could update instance props and state here,
233+
// but instead we rely on them being set during last render.
234+
// TODO: revisit this when we implement resuming.
233235
const snapshot = instance.getSnapshotBeforeUpdate(
234-
prevProps,
236+
finishedWork.elementType === finishedWork.type
237+
? prevProps
238+
: resolveDefaultProps(finishedWork.type, prevProps),
235239
prevState,
236240
);
237241
if (__DEV__) {
@@ -345,16 +349,21 @@ function commitLifeCycles(
345349
if (finishedWork.effectTag & Update) {
346350
if (current === null) {
347351
startPhaseTimer(finishedWork, 'componentDidMount');
348-
instance.props = finishedWork.memoizedProps;
349-
instance.state = finishedWork.memoizedState;
352+
// We could update instance props and state here,
353+
// but instead we rely on them being set during last render.
354+
// TODO: revisit this when we implement resuming.
350355
instance.componentDidMount();
351356
stopPhaseTimer();
352357
} else {
353-
const prevProps = current.memoizedProps;
358+
const prevProps =
359+
finishedWork.elementType === finishedWork.type
360+
? current.memoizedProps
361+
: resolveDefaultProps(finishedWork.type, current.memoizedProps);
354362
const prevState = current.memoizedState;
355363
startPhaseTimer(finishedWork, 'componentDidUpdate');
356-
instance.props = finishedWork.memoizedProps;
357-
instance.state = finishedWork.memoizedState;
364+
// We could update instance props and state here,
365+
// but instead we rely on them being set during last render.
366+
// TODO: revisit this when we implement resuming.
358367
instance.componentDidUpdate(
359368
prevProps,
360369
prevState,
@@ -365,8 +374,9 @@ function commitLifeCycles(
365374
}
366375
const updateQueue = finishedWork.updateQueue;
367376
if (updateQueue !== null) {
368-
instance.props = finishedWork.memoizedProps;
369-
instance.state = finishedWork.memoizedState;
377+
// We could update instance props and state here,
378+
// but instead we rely on them being set during last render.
379+
// TODO: revisit this when we implement resuming.
370380
commitUpdateQueue(
371381
finishedWork,
372382
updateQueue,

packages/react-reconciler/src/ReactFiberLazyComponent.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,21 @@ import type {LazyComponent, Thenable} from 'shared/ReactLazyComponent';
1212
import {Resolved, Rejected, Pending} from 'shared/ReactLazyComponent';
1313
import warning from 'shared/warning';
1414

15+
export function resolveDefaultProps(Component: any, baseProps: Object): Object {
16+
if (Component && Component.defaultProps) {
17+
// Resolve default props. Taken from ReactElement
18+
const props = Object.assign({}, baseProps);
19+
const defaultProps = Component.defaultProps;
20+
for (let propName in defaultProps) {
21+
if (props[propName] === undefined) {
22+
props[propName] = defaultProps[propName];
23+
}
24+
}
25+
return props;
26+
}
27+
return baseProps;
28+
}
29+
1530
export function readLazyComponentType<T>(lazyComponent: LazyComponent<T>): T {
1631
const status = lazyComponent._status;
1732
const result = lazyComponent._result;

packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js

Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,185 @@ describe('ReactLazy', () => {
282282
expect(root).toMatchRenderedOutput('SiblingB');
283283
});
284284

285+
it('sets defaultProps for modern lifecycles', async () => {
286+
class C extends React.Component {
287+
static defaultProps = {text: 'A'};
288+
state = {};
289+
290+
static getDerivedStateFromProps(props) {
291+
ReactTestRenderer.unstable_yield(
292+
`getDerivedStateFromProps: ${props.text}`,
293+
);
294+
return null;
295+
}
296+
297+
constructor(props) {
298+
super(props);
299+
ReactTestRenderer.unstable_yield(`constructor: ${this.props.text}`);
300+
}
301+
302+
componentDidMount() {
303+
ReactTestRenderer.unstable_yield(
304+
`componentDidMount: ${this.props.text}`,
305+
);
306+
}
307+
308+
componentDidUpdate(prevProps) {
309+
ReactTestRenderer.unstable_yield(
310+
`componentDidUpdate: ${prevProps.text} -> ${this.props.text}`,
311+
);
312+
}
313+
314+
componentWillUnmount() {
315+
ReactTestRenderer.unstable_yield(
316+
`componentWillUnmount: ${this.props.text}`,
317+
);
318+
}
319+
320+
shouldComponentUpdate(nextProps) {
321+
ReactTestRenderer.unstable_yield(
322+
`shouldComponentUpdate: ${this.props.text} -> ${nextProps.text}`,
323+
);
324+
return true;
325+
}
326+
327+
getSnapshotBeforeUpdate(prevProps) {
328+
ReactTestRenderer.unstable_yield(
329+
`getSnapshotBeforeUpdate: ${prevProps.text} -> ${this.props.text}`,
330+
);
331+
return null;
332+
}
333+
334+
render() {
335+
return <Text text={this.props.text + this.props.num} />;
336+
}
337+
}
338+
339+
const LazyClass = lazy(() => fakeImport(C));
340+
341+
const root = ReactTestRenderer.create(
342+
<Suspense fallback={<Text text="Loading..." />}>
343+
<LazyClass num={1} />
344+
</Suspense>,
345+
{
346+
unstable_isConcurrent: true,
347+
},
348+
);
349+
350+
expect(root).toFlushAndYield(['Loading...']);
351+
expect(root).toMatchRenderedOutput(null);
352+
353+
await Promise.resolve();
354+
355+
expect(root).toFlushAndYield([
356+
'constructor: A',
357+
'getDerivedStateFromProps: A',
358+
'A1',
359+
'componentDidMount: A',
360+
]);
361+
362+
root.update(
363+
<Suspense fallback={<Text text="Loading..." />}>
364+
<LazyClass num={2} />
365+
</Suspense>,
366+
);
367+
expect(root).toFlushAndYield([
368+
'getDerivedStateFromProps: A',
369+
'shouldComponentUpdate: A -> A',
370+
'A2',
371+
'getSnapshotBeforeUpdate: A -> A',
372+
'componentDidUpdate: A -> A',
373+
]);
374+
expect(root).toMatchRenderedOutput('A2');
375+
376+
root.update(
377+
<Suspense fallback={<Text text="Loading..." />}>
378+
<LazyClass num={3} />
379+
</Suspense>,
380+
);
381+
expect(root).toFlushAndYield([
382+
'getDerivedStateFromProps: A',
383+
'shouldComponentUpdate: A -> A',
384+
'A3',
385+
'getSnapshotBeforeUpdate: A -> A',
386+
'componentDidUpdate: A -> A',
387+
]);
388+
expect(root).toMatchRenderedOutput('A3');
389+
});
390+
391+
it('sets defaultProps for legacy lifecycles', async () => {
392+
class C extends React.Component {
393+
static defaultProps = {text: 'A'};
394+
state = {};
395+
396+
UNSAFE_componentWillMount() {
397+
ReactTestRenderer.unstable_yield(
398+
`UNSAFE_componentWillMount: ${this.props.text}`,
399+
);
400+
}
401+
402+
UNSAFE_componentWillUpdate(nextProps) {
403+
ReactTestRenderer.unstable_yield(
404+
`UNSAFE_componentWillUpdate: ${this.props.text} -> ${nextProps.text}`,
405+
);
406+
}
407+
408+
UNSAFE_componentWillReceiveProps(nextProps) {
409+
ReactTestRenderer.unstable_yield(
410+
`UNSAFE_componentWillReceiveProps: ${this.props.text} -> ${
411+
nextProps.text
412+
}`,
413+
);
414+
}
415+
416+
render() {
417+
return <Text text={this.props.text + this.props.num} />;
418+
}
419+
}
420+
421+
const LazyClass = lazy(() => fakeImport(C));
422+
423+
const root = ReactTestRenderer.create(
424+
<Suspense fallback={<Text text="Loading..." />}>
425+
<LazyClass num={1} />
426+
</Suspense>,
427+
);
428+
429+
expect(ReactTestRenderer).toHaveYielded(['Loading...']);
430+
expect(root).toFlushAndYield([]);
431+
expect(root).toMatchRenderedOutput('Loading...');
432+
433+
await Promise.resolve();
434+
435+
root.update(
436+
<Suspense fallback={<Text text="Loading..." />}>
437+
<LazyClass num={2} />
438+
</Suspense>,
439+
);
440+
expect(ReactTestRenderer).toHaveYielded([
441+
'UNSAFE_componentWillMount: A',
442+
'A1',
443+
'UNSAFE_componentWillReceiveProps: A -> A',
444+
'UNSAFE_componentWillUpdate: A -> A',
445+
'A2',
446+
]);
447+
expect(root).toFlushAndYield([]);
448+
expect(root).toMatchRenderedOutput('A2');
449+
450+
root.update(
451+
<Suspense fallback={<Text text="Loading..." />}>
452+
<LazyClass num={3} />
453+
</Suspense>,
454+
);
455+
expect(ReactTestRenderer).toHaveYielded([
456+
'UNSAFE_componentWillReceiveProps: A -> A',
457+
'UNSAFE_componentWillUpdate: A -> A',
458+
'A3',
459+
]);
460+
expect(root).toFlushAndYield([]);
461+
expect(root).toMatchRenderedOutput('A3');
462+
});
463+
285464
it('includes lazy-loaded component in warning stack', async () => {
286465
const LazyFoo = lazy(() => {
287466
ReactTestRenderer.unstable_yield('Started loading');

0 commit comments

Comments
 (0)