Skip to content

Commit 3b190a6

Browse files
acdliteAndarist
acdlite
and
Andarist
committed
forwardRef et al shouldn't affect if props reused
We don't have strong guarantees that the props object is referentially equal during updates where we can't bail out anyway — like if the props are shallowly equal, but there's a local state or context update in the same batch. However, as a principle, we should aim to make the behavior consistent across different ways of memoizing a component. For example, React.memo has a different internal Fiber layout if you pass a normal function component (SimpleMemoComponent) versus if you pass a different type like forwardRef (MemoComponent). But this is an implementation detail. Wrapping a component in forwardRef (or React.lazy, etc) shouldn't affect whether the props object is reused during a bailout. Co-authored-by: Mateusz Burzyński <[email protected]>
1 parent bd08137 commit 3b190a6

3 files changed

Lines changed: 189 additions & 0 deletions

File tree

‎packages/react-reconciler/src/ReactFiberBeginWork.new.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,24 @@ function updateSimpleMemoComponent(
598598
(__DEV__ ? workInProgress.type === current.type : true)
599599
) {
600600
didReceiveUpdate = false;
601+
602+
// The props are shallowly equal. Reuse the previous props object, like we
603+
// would during a normal fiber bailout.
604+
//
605+
// We don't have strong guarantees that the props object is referentially
606+
// equal during updates where we can't bail out anyway — like if the props
607+
// are shallowly equal, but there's a local state or context update in the
608+
// same batch.
609+
//
610+
// However, as a principle, we should aim to make the behavior consistent
611+
// across different ways of memoizing a component. For example, React.memo
612+
// has a different internal Fiber layout if you pass a normal function
613+
// component (SimpleMemoComponent) versus if you pass a different type
614+
// like forwardRef (MemoComponent). But this is an implementation detail.
615+
// Wrapping a component in forwardRef (or React.lazy, etc) shouldn't
616+
// affect whether the props object is reused during a bailout.
617+
workInProgress.pendingProps = nextProps = prevProps;
618+
601619
if (!checkScheduledUpdateOrContext(current, renderLanes)) {
602620
// The pending lanes were cleared at the beginning of beginWork. We're
603621
// about to bail out, but there might be other lanes that weren't

‎packages/react-reconciler/src/ReactFiberBeginWork.old.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,24 @@ function updateSimpleMemoComponent(
598598
(__DEV__ ? workInProgress.type === current.type : true)
599599
) {
600600
didReceiveUpdate = false;
601+
602+
// The props are shallowly equal. Reuse the previous props object, like we
603+
// would during a normal fiber bailout.
604+
//
605+
// We don't have strong guarantees that the props object is referentially
606+
// equal during updates where we can't bail out anyway — like if the props
607+
// are shallowly equal, but there's a local state or context update in the
608+
// same batch.
609+
//
610+
// However, as a principle, we should aim to make the behavior consistent
611+
// across different ways of memoizing a component. For example, React.memo
612+
// has a different internal Fiber layout if you pass a normal function
613+
// component (SimpleMemoComponent) versus if you pass a different type
614+
// like forwardRef (MemoComponent). But this is an implementation detail.
615+
// Wrapping a component in forwardRef (or React.lazy, etc) shouldn't
616+
// affect whether the props object is reused during a bailout.
617+
workInProgress.pendingProps = nextProps = prevProps;
618+
601619
if (!checkScheduledUpdateOrContext(current, renderLanes)) {
602620
// The pending lanes were cleared at the beginning of beginWork. We're
603621
// about to bail out, but there might be other lanes that weren't

‎packages/react-reconciler/src/__tests__/ReactMemo-test.js‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,159 @@ describe('memo', () => {
179179
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
180180
});
181181

182+
it('consistent behavior for reusing props object across different function component types', async () => {
183+
// This test is a bit complicated because it relates to an
184+
// implementation detail. We don't have strong guarantees that the props
185+
// object is referentially equal during updates where we can't bail
186+
// out anyway — like if the props are shallowly equal, but there's a
187+
// local state or context update in the same batch.
188+
//
189+
// However, as a principle, we should aim to make the behavior
190+
// consistent across different ways of memoizing a component. For
191+
// example, React.memo has a different internal Fiber layout if you pass
192+
// a normal function component (SimpleMemoComponent) versus if you pass
193+
// a different type like forwardRef (MemoComponent). But this is an
194+
// implementation detail. Wrapping a component in forwardRef (or
195+
// React.lazy, etc) shouldn't affect whether the props object is reused
196+
// during a bailout.
197+
//
198+
// So this test isn't primarily about asserting a particular behavior
199+
// for reusing the props object; it's about making sure the behavior
200+
// is consistent.
201+
202+
const {useEffect, useState} = React;
203+
204+
let setSimpleMemoStep;
205+
const SimpleMemo = React.memo(props => {
206+
const [step, setStep] = useState(0);
207+
setSimpleMemoStep = setStep;
208+
209+
const prevProps = React.useRef(props);
210+
useEffect(() => {
211+
if (props !== prevProps.current) {
212+
prevProps.current = props;
213+
Scheduler.unstable_yieldValue('Props changed [SimpleMemo]');
214+
}
215+
}, [props]);
216+
217+
return <Text text={`SimpleMemo [${props.prop}${step}]`} />;
218+
});
219+
220+
let setComplexMemo;
221+
const ComplexMemo = React.memo(
222+
React.forwardRef((props, ref) => {
223+
const [step, setStep] = useState(0);
224+
setComplexMemo = setStep;
225+
226+
const prevProps = React.useRef(props);
227+
useEffect(() => {
228+
if (props !== prevProps.current) {
229+
prevProps.current = props;
230+
Scheduler.unstable_yieldValue('Props changed [ComplexMemo]');
231+
}
232+
}, [props]);
233+
234+
return <Text text={`ComplexMemo [${props.prop}${step}]`} />;
235+
}),
236+
);
237+
238+
let setMemoWithIndirectionStep;
239+
const MemoWithIndirection = React.memo(props => {
240+
return <Indirection props={props} />;
241+
});
242+
function Indirection({props}) {
243+
const [step, setStep] = useState(0);
244+
setMemoWithIndirectionStep = setStep;
245+
246+
const prevProps = React.useRef(props);
247+
useEffect(() => {
248+
if (props !== prevProps.current) {
249+
prevProps.current = props;
250+
Scheduler.unstable_yieldValue(
251+
'Props changed [MemoWithIndirection]',
252+
);
253+
}
254+
}, [props]);
255+
256+
return <Text text={`MemoWithIndirection [${props.prop}${step}]`} />;
257+
}
258+
259+
function setLocalUpdateOnChildren(step) {
260+
setSimpleMemoStep(step);
261+
setMemoWithIndirectionStep(step);
262+
setComplexMemo(step);
263+
}
264+
265+
function App({prop}) {
266+
return (
267+
<>
268+
<SimpleMemo prop={prop} />
269+
<ComplexMemo prop={prop} />
270+
<MemoWithIndirection prop={prop} />
271+
</>
272+
);
273+
}
274+
275+
const root = ReactNoop.createRoot();
276+
await act(async () => {
277+
root.render(<App prop="A" />);
278+
});
279+
expect(Scheduler).toHaveYielded([
280+
'SimpleMemo [A0]',
281+
'ComplexMemo [A0]',
282+
'MemoWithIndirection [A0]',
283+
]);
284+
285+
// Demonstrate what happens when the props change
286+
await act(async () => {
287+
root.render(<App prop="B" />);
288+
});
289+
expect(Scheduler).toHaveYielded([
290+
'SimpleMemo [B0]',
291+
'ComplexMemo [B0]',
292+
'MemoWithIndirection [B0]',
293+
'Props changed [SimpleMemo]',
294+
'Props changed [ComplexMemo]',
295+
'Props changed [MemoWithIndirection]',
296+
]);
297+
298+
// Demonstrate what happens when the prop object changes but there's a
299+
// bailout because all the individual props are the same.
300+
await act(async () => {
301+
root.render(<App prop="B" />);
302+
});
303+
// Nothing re-renders
304+
expect(Scheduler).toHaveYielded([]);
305+
306+
// Demonstrate what happens when the prop object changes, it bails out
307+
// because all the props are the same, but we still render the
308+
// children because there's a local update in the same batch.
309+
await act(async () => {
310+
root.render(<App prop="B" />);
311+
setLocalUpdateOnChildren(1);
312+
});
313+
// The components should re-render with the new local state, but none
314+
// of the props objects should have changed
315+
expect(Scheduler).toHaveYielded([
316+
'SimpleMemo [B1]',
317+
'ComplexMemo [B1]',
318+
'MemoWithIndirection [B1]',
319+
]);
320+
321+
// Do the same thing again. We should still reuse the props object.
322+
await act(async () => {
323+
root.render(<App prop="B" />);
324+
setLocalUpdateOnChildren(2);
325+
});
326+
// The components should re-render with the new local state, but none
327+
// of the props objects should have changed
328+
expect(Scheduler).toHaveYielded([
329+
'SimpleMemo [B2]',
330+
'ComplexMemo [B2]',
331+
'MemoWithIndirection [B2]',
332+
]);
333+
});
334+
182335
it('accepts custom comparison function', async () => {
183336
function Counter({count}) {
184337
return <Text text={count} />;

0 commit comments

Comments
 (0)