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(17041): use react-fast-compare to compare props update in DataTable #17056

Merged

Conversation

yaoterry
Copy link
Contributor

Closes #17041

This is use react-fast-compare to compare the updated properties in DataTable's shouldComponentUpdate method, use lodash.isEqual will hurt performance issue from: facebook/react#19811

Changelog

New

  • {{new thing}}

Changed

  • replace the lodash.isequal with react-fast-compare to improve performance, otherwise it will break the DataTable re-render unsafely under some circumstance.

Removed

  • {{removed thing}}

Testing / Reviewing

this is just a 3rd party package replacement, we just need to make sure the DataTable can be rendered properly.

@yaoterry yaoterry requested a review from a team as a code owner July 29, 2024 08:12
Copy link

netlify bot commented Jul 29, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 3cffe00
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66a751ba14950a00084c0113
😎 Deploy Preview https://deploy-preview-17056--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 29, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 3cffe00
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66a751bac455ce0008bd93c5
😎 Deploy Preview https://deploy-preview-17056--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@riddhybansal
Copy link
Contributor

@tay1orjones since you were there in the conversation of this issue, What do you think this optimise the performance? If yes, should we try this with other components as well, i.e, using isEqual for element comparing in components like multiselect, filterable multiselect. Because there are issues we can generally see with firefox in many components.

@tay1orjones
Copy link
Member

@riddhybansal Yeah this looks great, I think we can use it anywhere we use lodash's isEqual.

Copy link
Contributor

@riddhybansal riddhybansal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for contributing !! This is awesome 🚀

@riddhybansal riddhybansal added this pull request to the merge queue Jul 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 29, 2024
@riddhybansal riddhybansal added this pull request to the merge queue Jul 29, 2024
Merged via the queue into carbon-design-system:main with commit 930d97b Jul 29, 2024
22 checks passed
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.

[Bug]: firefox is slow bug by the lodash.isEqual in DataTable with react component children in rows.
3 participants