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

wrong message when useMemo re-renders #68

Closed
vzaidman opened this issue Oct 16, 2019 · 22 comments
Closed

wrong message when useMemo re-renders #68

vzaidman opened this issue Oct 16, 2019 · 22 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@vzaidman
Copy link
Collaborator

In the following case:

const A = () => {
   const obj = useMemo(() => ({
     // something big
  }), [a, b, c, d, e]);

 return <Child  obj={obj} />;
}

when the hook useMemo re-creates obj even if it's deps deep equals, the message says A was re-rendered because if it, where in reality useMemo re-generated obj without a reason, but A is not re-rendered because of this.

@vzaidman
Copy link
Collaborator Author

vzaidman commented Nov 7, 2019

removed useMemo from being concidered as a reason for re-renders

@vzaidman vzaidman closed this as completed Nov 7, 2019
@o0x2a
Copy link

o0x2a commented Oct 13, 2020

It seems like this issue is still present, here is one for useMemo even if has empty [] dependencies list.
Screenshot 2020-10-13 at 16 58 22

@o0x2a
Copy link

o0x2a commented Oct 13, 2020

Can you confirm this issue? Should this ticket re-opened? @vzaidman

@vzaidman
Copy link
Collaborator Author

are you using the latest version?

@vzaidman
Copy link
Collaborator Author

@Hypnosphi hey!
do you remember why we did this?
Who do we even care if useMemo/useCallback returns a deep equals result?

@o0x2a
Copy link

o0x2a commented Oct 14, 2020

I use v4.3.2, the useMemo is used inside own custom hook, and the custom hook is not tracked by wdyr.

@Hypnosphi
Copy link
Contributor

@vzaidman because we will most likely pass it as a prop after all

@Hypnosphi
Copy link
Contributor

But I agree that those shouldn't be considered reasons for rerender of the component that uses them. I think we can only update dependenciesMap in them, without tracking hook changes

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Oct 14, 2020

To be fair, useMemo already was there at the moment of #147

@vzaidman
Copy link
Collaborator Author

OK! I just wanted to make sure.

@vzaidman
Copy link
Collaborator Author

@Hypnosphi now i also see that

dependenciesMap.set(hookResult, get(args, dependenciesPath))

saves the deps of the hook per hook result.

but if a hook simply returns an object that was created anywhere else it might create false positives.

for this reason I will only keep useCallback and remove useMemo from participating in dependenciesMap.

it's rare to return functions from useMemo anyway...

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Oct 14, 2020

not so rare:

const throttledCallback = useMemo(() => throttle(callback, timeout), [callback, timeout])

Please don't break this usecase.

What are usecases for returning a function created somewhere else by the way? Why does one even need useMemo for that?

@Hypnosphi
Copy link
Contributor

Also, dependenciesMap can't create false positives, only false negatives: it marks some functions as deeply different

@vzaidman
Copy link
Collaborator Author

gotcha i'll keep it inside.

By the way the throttle example is an anti pattern based on React 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.

@Hypnosphi
Copy link
Contributor

What is the recommended way to do that?

@vzaidman
Copy link
Collaborator Author

i think

const [throttledCallback] = useState(() => throttle(callback, timeout))

@vzaidman
Copy link
Collaborator Author

@Hypnosphi
Copy link
Contributor

What if both callback and timeout are dynamic?

@vzaidman
Copy link
Collaborator Author

Probably a longer and uglier hook using useRef...

@vzaidman
Copy link
Collaborator Author

this guy built a whole library around it:
https://github.com/alexreardon/use-memo-one

@vzaidman
Copy link
Collaborator Author

@Code-guru @Hypnosphi
released a fix in version v5.0.0-rc.1

@vzaidman
Copy link
Collaborator Author

i'll release version 5 very soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants