Fixed an issue with simple memo components being updated with new props during context change

[Andarist]

This is not ready to be merged yet because it only provides a failing test, I'd like to work on fixing this issue - but I would really appreciate some guidance with this. I've tried to understand what exactly is happening (wanted to send a PR with a fix already in it 😉 ), but failed to do so - need to dig deeper.

My current conclusion is that this fails because of the "simple memo" optimization - workLoop doesnt see this memo element at all, it "jumps" from working on Outer directly to Inner which has fresh pendingProps which are used as argument for working on Inner as it correctly has to rerender itself because it reads changed context. So it doesnt bail out on finished work (correctly) and commits used new props as memoized ones later.

Why this matters?
Because it's inconsistent with "regular" memo component. Conceptually from the user's perspective React.memo(Component) & React.memo(Component, shallowEqual) should behave exactly the same (but they dont)

I've prepared a demo to showcase the problem - https://codesandbox.io/s/jp21pwzrv9

[gaearon]

Can you file an issue too pls?

[Andarist]

Done - #14878

[gaearon]

Is this the only observable difference? Can you rewrite this test to check render count rather than whether props are referentially equal? Why does this matter at all?

[Andarist]

This checks the render count - see renderCountRef. I can't just count renders directly inside Inner, because it has to render (it reads changed context).

Why this matters?
Because it's inconsistent with "regular" memo component. Conceptually from the user's perspective React.memo(Component) & React.memo(Component, shallowEqual) should behave exactly the same (but they dont).

It makes caching assumptions less predictable. While this test is ofc artificial I've encountered this problem when implementing some derived state caching (based on both context & props) in a real world application. It caught me completely off guard because I was relaying on the fact that React.memo should always provide me same props (referentially equal) and I've only checked against that - skipping shallowEqual because I was trusting React to perform this one on my behalf.

[gaearon]

Can you change the test to rely on props.something rather than props itself? Relying on referential identity of props seems a bit like assuming too much to me

[Andarist]

Comparing against props referential identity isnt that unusual - https://github.com/reduxjs/react-redux/blob/fac9ad19d17000f5b9fdd52fc9843225b807845b/src/components/connectAdvanced.js#L142 .

I can't change this test to rely on props.something because this issue is specifically about the fact that props are not referentially equal in this case (sorry if I havent made that clear before). Anything on the props will be referentially equal as it pass shallowEqual test.

My point is that the current behaviour is inconsistent here and IMHO that's not a good thing because it makes the mental model about this a little bit unreliable.

Have you checked out my codesandbox which compares React.memo(Component) & React.memo(Component, shallowEqual)? I strongly believe that they should behave exactly the same and should not be distinguishable from user's perspective, as React.memo implementation conceptually looks like:

React.memo = (Component, compare = shallowEqual) => {
 // ...
}

Therefore supplying the second argument implicitly (relaying on default params) and explicitly should result in the very same observable behaviour.

When I've encountered this issue in my code I've supplied shallowEqual as 2nd argument by hand, just so I could put a debugger statement quicker in it's implementation to see why my component is rerendering and to my surprise it has fixed "the issue" which was at very least really surprising (knowing that React.memo uses shallowEqual by default)

[Andarist]

@aweary yeah, that's exactly my conclusion - simple memo's path is working differently

[aweary]

It's certainly inconsistent, but I'm not sure React makes any guarantees about props being referentially equal between updates, so it's not clear what the correct behavior here is. Ideally I think we'd warn about using props as a dependency.

[Andarist]

It's certainly inconsistent, but I'm not sure React makes any guarantees about props being referentially equal between updates, so it's not clear what the correct behavior here is.

My main problem here is not even about props not being referentially equal between updates (although I still believe it's quite a valid assumption for anyone using React.memo / PureComponent) but about the inconsistency.

As a developer I would chose a quirky, surprising behaviour (but stable & consistent) over inconsistent any day, because consistency helps me think - it reassures me about my knowledge and expectations, whereas inconsistent behaviour leads me to questioning things I shouldnt do. If a single behaviour is inconsistent how can I be sure that others truly are? Especially when debugging it's preferable to have a clear model in mind.

So in my opinion referential equality here should be a guaranteed at all times or not guaranteed at all. React.memo(C), React.memo(C, shallowEqual) & var c = React.memo(C); c.defaultProps = {}; should share the very same behaviour at all times. Warning about props being used as dependency would be somewhat helpful, but wouldnt cover all situations - if you don't want to give this guarantee to your users I would advocate for rerendering each memo with new props twice in dev mode. That would consistently force people to stop relaying on this unreliable behaviour.

Currently it's just too easy to rely on it, not being aware about this subtelty then throw a useContext inside such render and be kinda screwed, as in many times developer might not notice the difference but might regress some app's behaviour.

Just to reiterate on the importance of this as I'm not sure what your current stance on the matter is. The important question is if you would be OK with this (quirky behaviour, not props identity) knowing about it at the time of implementation? I suppose not - and that you would strive for implementing consistent behaviour.

While thinking about it some more I was also thinking about how props correspond to state & context as those 3 are React's primitives for holding data. In hooks world props are slightly different than state and context because latter hold a single value while props is a collection of values. However I still don't see much of a problem comparing them as mentally they are just slots for values for me. So to continue on this - state & context (and state in class world) have this guarantee, we can be sure that they hold the very same value (referentially equal) if there was no trigger to change on a particular value. I don't quite see how props should deviate from that - even if they are collection of data. Seems a valid assumption to me that in case of memo compare returning false is the only trigger that should swap the props value.

And also going slightly further with this - wouldnt it be user's expected assumption that in case of React.memo(C, () => false) they should always receive the same props object?

[aweary]

As a developer I would chose a quirky, surprising behaviour (but stable & consistent) over inconsistent any day

I agree, the inconsistency isn't great. I don't think it's intentional either and it could represent a bug somewhere. The open question here is, which one is correct? Memoizing the props seems like a nice idea at first but can we commit to that guarantee? It's also not consistent with the rest of React.

although I still believe it's quite a valid assumption for anyone using React.memo / PureComponent

PureComponent never relies on referential equality. Just like memo, it uses a shallow comparison internally.

react/packages/react-reconciler/src/ReactFiberClassComponent.js

Line 275 in 0e67969

!shallowEqual(oldProps, newProps) || !shallowEqual(oldState, newState)

You can see that with shouldComponentUpdate the nextProps isn't referentially equal to this.props, even if props are unchanged.

So relying on props === nextProps has never really worked, but we couldn't ever tell if users were relying on this. With useMemo we could potentially recognize that the rendering component is using its props object as a dependency and warn against it.

Currently it's just too easy to rely on it, not being aware about this subtelty then throw a useContext inside such render and be kinda screwed, as in many times developer might not notice the difference but might regress some app's behaviour.

Your app's behavior shouldn't depend on useMemo (or React.memo) preventing re-renders. From the API docs:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

So even though this is inconsistent, it shouldn't affect your app's behavior. If it does, your app is likely to break again in the future.

[Andarist]

I agree, the inconsistency isn't great. I don't think it's intentional either and it could represent a bug somewhere.

Agree on that 👍 Would be good at the very least to dig into this more to understand why exactly this happens to assess how much of a bug it might be.

PureComponent never relies on referential equality. Just like memo, it uses a shallow comparison internally.

Yeah - I know that. I understand both concepts and how they work. I was just stating that referential equality was kinda implied when using React.memo & React.PureComponent. I havent tested this thoroughly in the past, but this was always my mental model - that generally if any form of shouldComponentUpdate returns false I would receive cached props object (and according to my few tests now this is holds true to all other related cases). Because if not then any reads on props would be unsafe - in this particular case they are, but just because this happens for the default shallowEqual algorithm.

So even though this is inconsistent, it shouldn't affect your app's behavior. If it does, your app is likely to break again in the future.

That's good to know - gonna strive for this when writing component from now on, although I was using this merely as performance optimization anyway. I can imagine somebody using this in some other way though.

[Jessidhia]

I think you could technically fix this by changing https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberBeginWork.js#L455-L470 and https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberBeginWork.js#L391-L405 to set workInProgress.memoizedProps = current.memoizedProps / newChild.memoizedProps = currentChild.memoizedProps.

But should it be? 🤔

[Jessidhia]

You can just use useContext here, the readContext is if you need to access the context from outside the render pass.

[Andarist]

Oh, good to know! I was writing this test based on other test in this file and it has used this readContext thingy inside the render 🤔

[Jessidhia]

Rather, I assume most of these tests predate hooks. The code for the useContext hook is literally just this readContext, the only difference is it creates a node in the memoizedState linked list.

[Andarist]

I was considering similar patch but it didnt sound right - memoizedProps are only assigned right now in kinda post-update manner (from what I remember), so it would be rather hacky to assign to them here. Also it possibly wouldnt work (but I havent checked) as in this post-update place where memoizedProps are being assigned to they are given pendingProps, so while this probably would "fix" this particular case it would fail in similar way one update later (and we'd have to reassign nextProps here too which are being given to the function component).

I couldnt break the latter path anyhow though - it has worked fine.

The whole thing happens because workLoop never sees the simple memo component here, whereas it sees the regular ones (so in this case it never has a chance to bail out on it). It leads me to believe that this is some bug in how simple memo are handled when reconciling them during context update OR this is intentional optimization that it jumps directly from working on Outer to Inner and it that it has somewhat surprising effect on the nextProps given to Inner.

I'll dig into this further, but I still don't quite understand how fibers are being connected and how context change is being handled by React - I would appreciate any resources about those topics if you have any.

[Andarist]

Looking through experimental branch of react-redux it seems that props identity in memo case is something that might be expected by developers:
https://github.com/reduxjs/react-redux/blob/8122cd455775322d4d7be8fecb68bae0e7b5297c/src/components/connectAdvanced.js#L140-L147

This is not strong argument in favour of this behaviour, rather just pointing out that it isn't uncommon to assume this

cc @markerikson (might be of interest to you)

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[Andarist]

I still believe the issue is sound, and unless closed by React team member it shouldn't be treated as stale.

[stale]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[Andarist]

I still believe the issue is sound, and unless closed by React team member it shouldn't be treated as stale.

[gaearon]

Did you have a conclusion about what a fix could look like? I haven’t looked at this in depth yet.

[andrelmchaves]

@Andarist Could you confirm this is still an issue?

[gaearon]

I think I've looked at the original repro and it still repro'd in 18.

[Andarist]

Yes - the issue is still there, I've prepared an updated repro case here (using createRoot and StrictMode)

Did you have a conclusion about what a fix could look like? I haven’t looked at this in depth yet.

Sorry, I've missed this question from 2020. It's hard for me to tell because it's really depending on the internal implementation. For some reason the memo bailout doesn't happen here at all - maybe I can try to debug this again, since it might be way easier nowadays with Replay (❤️ )

[sizebot]

Comparing: d63cd97...bd13426

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.03%	131.43 kB	131.47 kB	+0.02%	42.05 kB	42.06 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.03%	136.70 kB	136.74 kB	+0.02%	43.64 kB	43.65 kB
facebook-www/ReactDOM-prod.classic.js	+0.04%	435.12 kB	435.27 kB	+0.04%	79.88 kB	79.91 kB
facebook-www/ReactDOM-prod.modern.js	+0.04%	420.12 kB	420.27 kB	+0.04%	77.51 kB	77.54 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.04%	435.12 kB	435.27 kB	+0.04%	79.88 kB	79.91 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against bd13426

[Andarist]

@gaearon I've spent some time yesterday with Replay here and I got some conclusions about the situation here - note that my conclusions might be completely off because I have limited knowledge about React internals.

From what I understand a "simple memo" is a memo that doesn't use a custom compare and it doesn't have any .defaultProps (note that with const ImplicitInner = React.memo(Inner); ImplicitInner.defaultProps = {} it starts to behave like the ExplicitInner cause then both hit the same code path, they both go into updateMemoComponent ).

I think that the "simple memo" stuff is just an internal optimization and that it "flattens" the fiber tree (?). The whole problem~ here happens during a context update and, from what I understand, a context update marks all consuming fibers~ as "dirty". When we update a component we must reconcile its children and while we can bail out on props equality we might still want to rerender a child if it consumes a context (this is determined by checkScheduledUpdateOrContext).

So conceptually the component must rerender when either its props or context change in this part of the algorithm (I don't know why state change isn't handled here or where it is actually handled). And the checkScheduledUpdateOrContext is required here to prevent false-positive bailouts that could result from just compare(prevProps, nextProps).

Going back to the whole "flattening" thing though - when we deal with non-simple memo it's not that fiber that has a "pending" context update, it's probably its "child". So in this situation, prev props are preserved on the inner fiber as the non-simple memo just bails out and the new pendingProps (?) are not committed to it. The wrapped component still rerenders because React visits the next unit of work~ that is this wrapped component since it has a pending context update.

In the case of a simple memo though it's the same fiber that does the memo check and that has a context update scheduled for it. So it ends up calling updateFunctionComponent because it can't bailout based on the memo check alone here (checkScheduledUpdateOrContext return true for it). And that makes the updateFunctionComponent call the wrapped component with a new props object.

I totally agree with you that you make it pretty clear that memo stuff is not a semantic guarantee. However, I still believe that it is a conceptual problem (a small one) because conceptually, from the user perspective, both of those components should behave in the very same way. I agree that we can't rely on the useMemo etc here in the general sense but I think that it should come with the same caching characteristics as its "complex" memo equivalent (or that the "complex" one should have the same characteristics as the "simple" one). I believe in the principle of least surprise and this is violated here (slightly).

I managed to fix this particular issue yesterday by recycling the prevProps object for the updateFunctionComponent call. However, I've realized today that this might not work for subsequent rerenders - and in fact, after I've added one additional update to the test case it has failed. So I've managed to fix this by setting the recycled object on the workInProgress.pendingProps - but that feels like a hack to me (but maybe it isn't?). I've tried to check if this has the same guarantees about the props object as in the "complex" memo case and it seems so far that it does (but maybe I've missed something). I've prepared this codesandbox and it still doesn't bust the useMemo cache even if we first update a context multiple times with the "Rerender!" button and then press "Explicit: set state" button repeatedly.

It would be great if you (React team) could decide if this is a legitimate issue from your PoV or if you want to close it. I can investigate this further and maybe try some other approaches if you'd have any suggestions - but it would be great if this could get classified as an issue first.

Also - even if this doesn't get classified as a bug it could still be seen as a potential perf improvement for some situations.

[gaearon]

Just to set expectations, this is pretty low in terms of priority since there's a bunch of semantic bugs we need to follow up on. But I'll add this to our triage list.

[Andarist]

No problem, it's good to know that it is still pending evaluation and that it might happen one day :P

[acdlite]

I agree with @gaearon that we don't have a strong guarantee that the props object is reused if there's a state or context change. Your code should be resilient either way. And as @aweary pointed out, the behavior is already inconsistent between class components (shouldComponentUpdate/PureComponent) and memo.

However I also agree with @Andarist that wrapping a memoized function component in forwardRef (or another level of memo) shouldn't affect whether the props object gets reused. SimpleMemoComponent is an implementation detail. In principle, there shouldn't be a behavior difference, regardless of what the behavior happens to be.

I have a slightly different preference for how to test this so I'll open a modified version of this PR where we can continue the discussion.

[acdlite]

• forwardRef et al shouldn't affect if props object is reused #24421

[acdlite]

Closed by #24421