-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add shouldNotifyObservers to ObservableState #3751
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
Add shouldNotifyObservers to ObservableState #3751
Conversation
@rcarver Thanks for looking into this! Are your TODOs still pending or is this ready to review? |
@stephencelis np! Still todo (switched the PR to a draft). Could you take a look at Any insights you can provide would be super helpful; I'll take another pass at it. |
@mbrandonw thanks for that. Demo still looks good.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This adds
shouldNotifyObservers
toObservableState
. It's basically copied fromPerception
, then merged withObservationRegistrar
TODO
testCopyMutation_WithPerturbation
Demo
https://github.com/rcarver/tca-sharing-observation-tests
The TCA case in this app shows that a noop assignment no longer causes a view update. This all started by working on observation for Shared, but I thought it might be informative to first refine this piece within TCA.