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

ContextProvider - wrap callbacks #1942

Merged
merged 4 commits into from
Jul 27, 2022
Merged

Conversation

Inbal-Tish
Copy link
Collaborator

No description provided.

}
};
}, [currentDate, onDateChanged, onMonthChange]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the main issue we'll have here, is the fact that currentData will change too often and will re-create the _setDate callback.
This is btw and known React issue, and they have plans to release a new hook called useEvent
You can read about it here

Anyway, for now I will approve the PR.
We can maybe think of a way to avoid re-creation of the callback for now, maybe keep the currentDate as a ref value as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'm familiar with useEvent. I removed the prevDate from the state, as there is no need for that, but I don't see an option removing the currentDate from the state as the UI needs to react to its changes (today button, selected date, etc.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh definitely, I actually meant to keep it both in state and in ref.
The ref is only meant for the setDate callback do you won't need to recreate it each time...
It's far from ideal but it can be a temp solution till we'll have something like useEvent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh. Got it.

@ethanshar ethanshar merged commit e808253 into master Jul 27, 2022
@Inbal-Tish Inbal-Tish deleted the infra/ContextProvider_performance branch July 31, 2022 07:29
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.

None yet

2 participants