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

Avoid key update for every re-render of ArticleContent with the same content #423

Closed
wants to merge 1 commit into from

Conversation

lingyuncai
Copy link

@lingyuncai lingyuncai commented Aug 29, 2024

This is to address #422.

Use useMemo() to avoid uuidv4() call on the same content, which introduced unnecessary re-renders of the same <li> element.

…content

Use `useMemo()` to avoid uuidv4() call on the same content, which introduced
unnecessary re-renders of the same <li> element.
@julienw
Copy link
Contributor

julienw commented Aug 30, 2024

This looks reasonable to me, but I wonder if the id prop could be generated instead wherever we create the items in the first place. I'd like @flashdesignory to chime in here.

Also you mention in #422 that nuxt has a similar issue where id is undefined. I believe this could be solved the same way.

@flashdesignory
Copy link
Contributor

@julienw - you are correct, the id should be coming from the data itself.
Looks like I didn't add an id to the article content.

I'll update the data and open a pr

Thanks @lingyuncai for surfacing the issue

@lingyuncai
Copy link
Author

Hi @julienw and @flashdesignory, thanks for the review!

Storing the id prop in locally generated data is indeed a more clean way to achieve the goal, while it also unifies the behavior across the Next/Nuxt workloads.

Thanks again for the effort on this :)

@rniwa
Copy link
Member

rniwa commented Sep 4, 2024

Is this PR now obsolete given #426 and #427?

@lingyuncai
Copy link
Author

Yes, closing now.

@lingyuncai lingyuncai closed this Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants