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

Defer notifications while performing action #247

Merged
merged 13 commits into from
Jan 4, 2024
Merged

Conversation

ShirShintel
Copy link
Contributor

@ShirShintel ShirShintel commented Dec 28, 2023

  • Add deferSubscriberNotifications Function that defers subscribers (and observables) notification until the end of some action. This is necessary when running a flow that creates intermediate states we don't want to react to.

  • To handle the conflict between deferSubscriberNotifications and flush, add optional config to the flush function:

    • excecutionType: default- meaning flush would happen right after we stop deferring notifications. Flush when not deferring notifications stays the same.
    • excecutionType: scheduled- meaning flush would notify all without canceling render and requesting animation frame, regardless if notifications are currently being deferred.
    • excecutionType: immediate - meaning flush would happen immediately regardless if notifications are currently being deferred.

In general, the deferSubscriberNotifications solution is:

  • deferSubscriberNotifications receives some action we would like to perform while deferring notifications.
  • If notifications are already deferred, run action without doing anything else (to support nesting deferSubscriberNotifications)
  • Set deferNotifications flag to true before running the action
  • When calling notifyAll while deferNotifications is set to true, return without notifying
  • Finally, set deferNotifications back to false, and flush\notify if someone tried to flush (not immediate) or notify while deferring notifications.

@ShirShintel ShirShintel marked this pull request as ready for review December 28, 2023 15:06
Copy link
Contributor

@shirlynwix shirlynwix left a comment

Choose a reason for hiding this comment

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

Very nice!
See small suggestions

src/throttledStore.tsx Show resolved Hide resolved
test/connectWithShell.spec.tsx Outdated Show resolved Hide resolved
@ShirShintel ShirShintel merged commit 88454a6 into master Jan 4, 2024
1 check passed
@ShirShintel ShirShintel deleted the defer-notifications branch January 4, 2024 09:27
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