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

[core] Avoid useless columns preprocessing on Grid mount #2286

Closed
wants to merge 9 commits into from

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Aug 5, 2021

Avoid double useEffect run during grid init

Fixes #1562

@flaviendelangle flaviendelangle self-assigned this Aug 5, 2021
@flaviendelangle flaviendelangle added the core Infrastructure work going on behind the scenes label Aug 5, 2021
@flaviendelangle flaviendelangle added the on hold There is a blocker, we need to wait label Aug 6, 2021
@flaviendelangle flaviendelangle marked this pull request as ready for review August 10, 2021 08:02
@oliviertassinari
Copy link
Member

@flaviendelangle
Copy link
Member Author

I was not the main goal but nice 😄

@flaviendelangle flaviendelangle added performance and removed on hold There is a blocker, we need to wait labels Aug 10, 2021
@flaviendelangle
Copy link
Member Author

@oliviertassinari if #2344 has the same behavior, it is indeed simpler and I'm OK to close this one

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 24, 2021

@flaviendelangle I'm not exactly sure it has the same behavior. I looked at it because I was suspicious we could have a simpler logic/wanted to understand what was going on in this hook. I don't plan to push #2344 much further. If you can continue the exploration, and pick whoever works best, I think that it would be great.

@flaviendelangle flaviendelangle changed the base branch from master to next August 26, 2021 07:56
@flaviendelangle
Copy link
Member Author

After a few checks, the behavior seems to be very similar
If we see edge case not optimized with your version we'll see 👍
I'm closing mine and approving + merging yours

@oliviertassinari
Copy link
Member

Alright, at least, it easy to follow. Note that we have a few tests in the codebase that assert we don't render too many times. We could consider doing the same if we see regressions.

@flaviendelangle flaviendelangle deleted the perf-columns branch October 9, 2021 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkbox column appended instead of prepended in concurrent mode
2 participants