Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

memo equality check function overrides state #14972

Closed
minheq opened this Issue Feb 28, 2019 · 3 comments

Comments

Projects
None yet
3 participants
@minheq
Copy link

minheq commented Feb 28, 2019

Do you want to request a feature or report a bug?
BUG

What is the current behavior?
When a component is wrapped with React.memo and is provided 2nd argument - equality check function, it overrides state in the parent component

What is the expected behavior?
It should not override the state in parent component

Reproduction and detailed explanation of the bug is within this codesandbox:
https://codesandbox.io/s/4j6ownx8x0

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Codesandbox

Note: I spent few hours verifying that this is a genuine bug and not a due to my misunderstanding, but I apologize if it is the latter.

@iskandar17

This comment has been minimized.

Copy link

iskandar17 commented Feb 28, 2019

https://codesandbox.io/s/943916znlp
see what memo component renders here after each call

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Feb 28, 2019

Custom equality checks must include all props you care about. Including callbacks. (This is why generally you shouldn't supply your own comparison function — it's easy to make a mistake!)

The mistake you're making is that you're ignoring changes to props.onSelect. So the onSelect handler of both buttons "sees" the initial selectedStartDate state only (null). It's never updated. Therefore, if (!selectedStartDate) is always true and you always end up calling setSelectedStartDate.

If I fix your equality function, it works:

const equalityCheck = (prevProps, nextProps) =>
-  prevProps.comparedProp === nextProps.comparedProp;
+  prevProps.comparedProp === nextProps.comparedProp &&
+  prevProps.onSelect === nextProps.onSelect;

https://codesandbox.io/s/62961yk72z

You can also useCallback if you don't want to break memoization by always passing different functions. But in that case you need to make sure you don't close over anything that's not specified in deps. So you can't close over selectedStartDate in your check.

The most idiomatic fix to this is useReducer. It makes it easy to preserve callback identity while having more complicated state logic. Here's an example: https://codesandbox.io/s/62961yk72z.

@gaearon gaearon closed this Feb 28, 2019

@minheq

This comment has been minimized.

Copy link
Author

minheq commented Feb 28, 2019

Many thanks for your elaborate explanation! I understand it now, hopefully others who make this same mistake will find their answers here as well

and thanks @iskandar17 for chiming in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.