- Notifications
You must be signed in to change notification settings - Fork524
Proof of concept inverting subscription transforms#307
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
RE:#263This is a proof of concept for simplifying the subscription selection / skipping usage.```swift// Previously this:store.subscribe(subscriber) { $0.select { $0.testValue }.skip { $0 == $1 }}// Now can be this:store.select { $0.testValue } .skip { $0 == 1 } .subscribe(subscriber)```I've implemented the prototype in such a way that maintains backwards compatibility, which would allow us to deprecate the changes for a release or two before breaking compatibility. (if we ever wanted to break it to clean up the code)
codecov-io commentedNov 28, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #307 +/- ##===================================== Coverage 100% 100% ===================================== Files 5 5 Lines 160 181 +21 =====================================+ Hits 160 181 +21
Continue to review full report at Codecov.
|
TODO:- [ ] Filter & remove subscriptions without subscribers (need to walk to the end)- [ ] automaticallySkipRepeats (`extension StoreSubscriber where StoreSubscriberStateType: Equatable` doesn't work / isn't called)- [ ] unsubscribe- [ ] retain cycle tests
| } | ||
| } | ||
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.
Vertical Whitespace Violation: Limit vertical whitespace to a single empty line. Currently 2. (vertical_whitespace)
Trailing Newline Violation: Files should have a single trailing newline. (trailing_newline)
My latest commit here refactors Subscriptions now look like this: |
ReSwift/CoreTypes/Store.swift Outdated
| // subscriptions = subscriptions.filter { $0.subscriber != nil } | ||
| subscriptions.forEach{ | ||
| $0.newValues(oldState:oldValue, newState: state) | ||
| $0.notify(oldValue, state) |
DivineDominionDec 5, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Love this 👍 Much better method name
| openfunc subscription()->Subscription<State>{ | ||
| letsubscription=Subscription<State>( | ||
| initialState:self.state, | ||
| automaticallySkipsEquatable:self.subscriptionsAutomaticallySkipRepeats |
DivineDominionDec 5, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Why not use the same parameter name, one way or the other? If this is a breaking API change anyway, I’m d’accord with renaming subscriptionsAutomaticallySkipRepeats to subscriptionsAutomaticallySkipEquatable.
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.
I'm for changing that as well, I thinksubscriptionsAutomaticallySkipEquatable is much more clear as to what its doing.
| initialState:self.state, | ||
| automaticallySkipsEquatable:self.subscriptionsAutomaticallySkipRepeats | ||
| ) | ||
| self.subscriptions.append(subscription) |
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.
This means that store.subscription() already inserts a subscription, even though subscribe() is never called, right?
If so, I think we should change that to operate on a pending subscription that is only activated in the store on subscribe(). This is now more like a factory; the side effect can cause troubles. If you do not call append here, you do not run into problems when you grab a handle of the subscription during its transformation and intend to branch off in 2+ directions. That’d be similar to RxSwift.Observalbe stream transformations and its subscribe call, which is a prominent and rather well understood pattern.
ReSwift/CoreTypes/Subscription.swift Outdated
| publicfunc skipRepeats()->Subscription<State>{ | ||
| returnself.skipRepeats(==) | ||
| publicfunc subscribe<S:StoreSubscriber>(_ subscriber:S)where S.StoreSubscriberStateType==State{ | ||
| assert(self.subscriber==nil,"Subscriptions do not support multiple subscribers") |
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.
See comment above; not allowing multiple calls to subscribe() is unintuitive and makes the function a partially applied function, if I remember the term correctly, through the (debug build-only) assertion. I think this should be a precondition that affects client code, because if not, the behavior is supposed to be undefined.
but will not show any sign of a problem.
That being said, in accordance to the stuff above, I think it should be subscribe(store:) instead, with a PendingSubscription from store.subscription() being sugar to hold on to a Store and Subscription instance, and PendingSubscription.subscribe() simply passing the store to the subscription.
I really like the overall changes! That reads much nicer than the old block stuff. In general, I think the side effect of activating the subscription in the Store.subscription() factory problematic. I left a few comments with suggestions inside. I dunno how much time you want to spend on this; If you want me to propose a change in another branch for comparison instead of leaving the implementation to you, I’ll happily come up with code changes, just say Yes :) |
@DivineDominion I've tried to see if I can get anywhere with your suggestions but I keep running into walls, or the implementation gets confusing enough that its not worth it. I've put up my attempt inhttps://github.com/ReSwift/ReSwift/tree/mjarvis/invert-transform-2. My commit description has a bunch of notes / known issues etc. I would love for you to do a bit on this and see if you can simplify things |
Instead of passing the initial state along, we now fetch it at `subscribe` instead.This allows for the chance of a user holding onto the subscription for some time before adding a subscriber, and the state having changed in the meantime.
This allows us to take a given `Subscription` from `store.subscription()`, and apply multiple transforms to it, to give us a subscription "tree" of sorts.see `testSplittingStateSelector` for example.
| XCTAssertEqual(secondSubscriber.receivedValue,"TestName") | ||
| } | ||
| // TODO: Add tests for splitting after `select`, `skip`, and `only` |
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.
Todo Violation: TODOs should be avoided (Add tests for splitting after ...). (todo)
| // TODO: Add tests for splitting after `select`, `skip`, and `only` | ||
| // TODO: Add test for subscribing and splitting (I dont think this works right now) |
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.
Todo Violation: TODOs should be avoided (Add test for subscribing and s...). (todo)
@DivineDominion I've managed to partially implement tree-style subscription (allowing multiple |
@mjarvis Aha! So the store becomes a hub of subscriptions that each manages its own subscribers, right? Sounds clever, and reasonable. The concepts work, kind of: a subscription has many subscribers, and a store can have many ongoing subscriptions. (Maybe calling the intermediate products “subscription” conveys too much of a successful, finalized binding? What about StateUpdate for the intermediary objects, with the result of the final subscribe() returning a Subscription?) Have to check out the code on my Mac and run it tomorrow to see everything in context, esp. if a subscription with 0 subscribers removes itself from the store again and stuff like that :) What I can make sense of on GitHub looks good so far! |
I experimented with the changes and tried to fix some broken tests, then I split the Subscription type into subtypes to see if that makes getting a handle on things easier than the closure dance from before because then things have names in the stack trace. It kind of does, I find, but I have yet to fix any of the operators leaving a strong reference. Your approach was clever, with the notify/originalNotify being captured in the next operator’s closure. I’d like to reverse that, though, and will need to spend some more time on this tomorrow. Will see if I can manage to push the changes right now, which I forgot to do back at my desk :) |
Uh oh!
There was an error while loading.Please reload this page.
The intention of this PR is to simplify subscription selection & skipping, as well as resolve a number of bugs & negative performance implications of using
skipRepeatsin the current implementation.Fixes:
TODO:
extension StoreSubscriber where StoreSubscriberStateType: Equatabledoesn't work / isn't called when the storesubscriber itself is generic,class Bar<S>: StoreSubscriber)