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

fix:removing forwardLegendDataRef from hook dependency #2592

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZombieChowder11
Copy link

Related to issue #2591 #2569

This fix should remove the infinite re-renders when creating a custom Legend for responsive pies.

forwardLegendDataRef is not needed in the dependency of the useEffect hook

Copy link

vercel bot commented May 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nivo ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 17, 2024 0:00am

@plouc
Copy link
Owner

plouc commented May 17, 2024

@ZombieChowder11, thank you for submitting a fix.

I thought a ref's reference was stable, only current changes as I understand it.

@plouc
Copy link
Owner

plouc commented May 17, 2024

I found an interesting post about this: https://epicreact.dev/why-you-shouldnt-put-refs-in-a-dependency-array/, so it seems like this change is not going to fix the problem, not passing the ref and passing the ref object (not current) seems to lead to the same behavior.

@plouc
Copy link
Owner

plouc commented May 17, 2024

The problem might be about legend data changing.

@plouc
Copy link
Owner

plouc commented May 17, 2024

I think you could try locally the setup you shared in the issue you created with the updated code, creating a new story for example, also log in useEffect to see when it runs.

@ZombieChowder11
Copy link
Author

I think you could try locally the setup you shared in the issue you created with the updated code, creating a new story for example, also log in useEffect to see when it runs.

Something that came to my mind was to either use a useMemo hook or a callback for the legend data changing instead of a useEffect.

@plouc
Copy link
Owner

plouc commented May 17, 2024

Given the simplicity of the data for the legends, maybe deeply comparing the data would be better, rather than relying on other computations which might be necessary for the chart, but won't impact the legend's data. It would probably be less prone to infinite rendering, it's really easy to end up in this situation if you don't declare the props statically/memoize them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants