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

Optimize hooks a little #4

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Optimize hooks a little #4

wants to merge 5 commits into from

Conversation

JanBaryla
Copy link

@JanBaryla JanBaryla commented Mar 14, 2022

Overview

This is a tricky one because I don't know if it 100% works.

Basically the premise is to do two things:

  1. Remove the setTimeout logic out of useCoordinates and move it to the apply wrapper. Thanks to that, way less time is spent just waiting for the timeout to happen.
  2. Introduce a new useAreCoordinatesSelected hook to mitigate the need to run heavy comparison for every cell click in useCellIsSelected. Now it's going to return a boolean and should make things quicker for larger tables.

Risks

Point 1. is the risk-prone one. I've experimented with a couple of possible solutions, including using lodash.debounce instead of setTimeout, but then I ended up at what's in the PR and it still worked, so I stayed with that. I have not seen it do anything weird but then again, testing npm packages locally isn't an easy task.

If we vote against Point 1, Point 2 definitely seems like an easy win, and I'd be happy to cherry-pick just that change.

Testing

See if coordinates stay updated. Monitor the speed of useCellIsSelected.

@MichaelDarr
Copy link
Contributor

This looks really promising. I'm going to push a couple stylistic changes and do some testing, and maybe we can get this through!

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.

2 participants