forwardRef et al shouldn't affect if props reused (#24421)
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 <mateuszburzynski@gmail.com>
Andrew Clark committed
Apr 22, 2022 at 14:23 UTC
6d3b6d0f408136479d7298f1a14d79e8a12f0c65
3 files changed
+189
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+18
@@ -598,6 +598,24 @@ function updateSimpleMemoComponent(
598
(__DEV__ ? workInProgress.type === current.type : true)
599
) {
600
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
+
619
if (!checkScheduledUpdateOrContext(current, renderLanes)) {
620
// The pending lanes were cleared at the beginning of beginWork. We're
621
// about to bail out, but there might be other lanes that weren't
packages/react-reconciler/src/ReactFiberBeginWork.old.js
+18
@@ -598,6 +598,24 @@ function updateSimpleMemoComponent(
598
(__DEV__ ? workInProgress.type === current.type : true)
599
) {
600
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
+
619
if (!checkScheduledUpdateOrContext(current, renderLanes)) {
620
// The pending lanes were cleared at the beginning of beginWork. We're
621
// about to bail out, but there might be other lanes that weren't
packages/react-reconciler/src/__tests__/ReactMemo-test.js
+153
@@ -179,6 +179,159 @@ describe('memo', () => {
179
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
180
});
181
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
+
335
it('accepts custom comparison function', async () => {
336
function Counter({count}) {
337
return <Text text={count} />;