Proof of concept inverting subscription transforms #307

mjarvis · 2017-11-27T22:59:07Z

The intention of this PR is to simplify subscription selection & skipping, as well as resolve a number of bugs & negative performance implications of using skipRepeats in the current implementation.

Fixes:

// Previously this:
store.subscribe(subscriber) {
    $0.select { $0.testValue }
        .skip { $0 == $1 }
}
// Now is this:
store.subscription()
    .select { $0.testValue }
    .skip { $0 == 1 }
    .subscribe(subscriber)

TODO:

Filter & remove subscriptions without subscribers (need to walk to the end)
automaticallySkipRepeats (extension StoreSubscriber where StoreSubscriberStateType: Equatable doesn't work / isn't called when the storesubscriber itself is generic, class Bar<S>: StoreSubscriber)
unsubscribe
retain cycle tests

RE: #263 This 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 · 2017-11-28T01:16:13Z

Codecov Report

Merging #307 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #307   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         160    181   +21     
=====================================
+ Hits          160    181   +21

Impacted Files	Coverage Δ
ReSwift/CoreTypes/Store.swift	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e293951...6d76ac5. Read the comment docs.

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

houndci-bot · 2017-12-05T00:41:07Z

ReSwift/CoreTypes/StoreSubscriber.swift

        if let typedState = state as? StoreSubscriberStateType {
            newState(state: typedState)
        }
    }
 }
+
+


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)

mjarvis · 2017-12-05T00:41:22Z

My latest commit here refactors Subscription, breaking backwards compatibility.
The intention here is to make the Subscription system much easier to understand, code-wise, eliminating a number of confusing code paths & bugs.

Subscriptions now look like this:

store.subscription()
    .select { $0.testValue }
    .skip { $0 == 1 }
    .subscribe(subscriber)

DivineDominion · 2017-12-05T16:52:28Z

ReSwift/CoreTypes/Store.swift

            subscriptions.forEach {
-                $0.newValues(oldState: oldValue, newState: state)
+                $0.notify(oldValue, state)


Love this 👍 Much better method name

DivineDominion · 2017-12-05T16:53:35Z

ReSwift/CoreTypes/Store.swift

+    open func subscription() -> Subscription<State> {
+        let subscription = Subscription<State>(
+            initialState: self.state,
+            automaticallySkipsEquatable: self.subscriptionsAutomaticallySkipRepeats


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.

I'm for changing that as well, I think subscriptionsAutomaticallySkipEquatable is much more clear as to what its doing.

DivineDominion · 2017-12-05T16:58:31Z

ReSwift/CoreTypes/Store.swift

        )
+        self.subscriptions.append(subscription)


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.

DivineDominion · 2017-12-05T17:03:48Z

ReSwift/CoreTypes/Subscription.swift

-    public func skipRepeats() -> Subscription<State>{
-        return self.skipRepeats(==)
+    public func subscribe<S: StoreSubscriber>(_ subscriber: S) where S.StoreSubscriberStateType == State {
+        assert(self.subscriber == nil, "Subscriptions do not support multiple subscribers")


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.

DivineDominion · 2017-12-05T17:06:30Z

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 :)

mjarvis · 2017-12-05T19:43:04Z

@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 in https://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.

houndci-bot · 2017-12-05T20:07:14Z

ReSwiftTests/StoreSubscriberTests.swift

+        XCTAssertEqual(secondSubscriber.receivedValue, "TestName")
+    }
+
+    // TODO: Add tests for splitting after `select`, `skip`, and `only`


Todo Violation: TODOs should be avoided (Add tests for splitting after ...). (todo)

houndci-bot · 2017-12-05T20:09:46Z

ReSwiftTests/StoreSubscriberTests.swift

+
+    // TODO: Add tests for splitting after `select`, `skip`, and `only`
+
+    // TODO: Add test for subscribing and splitting (I dont think this works right now)


Todo Violation: TODOs should be avoided (Add test for subscribing and s...). (todo)

mjarvis · 2017-12-05T20:09:55Z

@DivineDominion I've managed to partially implement tree-style subscription (allowing multiple select/skip/only/subscribe on a given Subscription) (I put a couple TODO in the tests)
I think that covers most of your concerns above. (maybe? 😄)

DivineDominion · 2017-12-05T20:52:03Z

@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!

DivineDominion · 2017-12-06T19:17:08Z

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 :)

DivineDominion · 2017-12-06T19:38:40Z

@mjarvis here it is so far: https://github.com/ReSwift/ReSwift/tree/DivineDominion/invert-transform-types

mjarvis added Help Wanted Needs Discussion Work In Progress labels Nov 27, 2017

Refactor Subscription

e5d2aa8

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

houndci-bot reviewed Dec 5, 2017

View reviewed changes

mjarvis mentioned this pull request Dec 5, 2017

Fix for #298 #300

Merged

DivineDominion reviewed Dec 5, 2017

View reviewed changes

Resolve linting warnings

2d74598

Malcolm Jarvis added 3 commits December 5, 2017 11:51

Allow multiple subscribers

8804c09

Get initial state at subscribe call

66c49aa

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.

Allow splitting subscription chains

a4086f1

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.

houndci-bot reviewed Dec 5, 2017

View reviewed changes

Add todo note for subscribe then select split

6d76ac5

houndci-bot reviewed Dec 5, 2017

View reviewed changes

DivineDominion mentioned this pull request Jun 3, 2018

Add test example for double equatable check #259

Open

DivineDominion mentioned this pull request Feb 2, 2019

ReSwift 5.0 Release Plan #303

Closed

10 tasks

DivineDominion mentioned this pull request Aug 6, 2019

Invert subscription transforms in a reactive way (attempt No. 2) #420

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept inverting subscription transforms #307

Proof of concept inverting subscription transforms #307

mjarvis commented Nov 27, 2017 •

edited

codecov-io commented Nov 28, 2017 •

edited

houndci-bot Dec 5, 2017

mjarvis commented Dec 5, 2017

DivineDominion Dec 5, 2017 •

edited

DivineDominion Dec 5, 2017 •

edited

mjarvis Dec 5, 2017

DivineDominion Dec 5, 2017

DivineDominion Dec 5, 2017

DivineDominion commented Dec 5, 2017

mjarvis commented Dec 5, 2017

houndci-bot Dec 5, 2017

houndci-bot Dec 5, 2017

mjarvis commented Dec 5, 2017

DivineDominion commented Dec 5, 2017

DivineDominion commented Dec 6, 2017

DivineDominion commented Dec 6, 2017


		// TODO: Add tests for splitting after `select`, `skip`, and `only`

		// TODO: Add test for subscribing and splitting (I dont think this works right now)

Proof of concept inverting subscription transforms #307

Are you sure you want to change the base?

Proof of concept inverting subscription transforms #307

Conversation

mjarvis commented Nov 27, 2017 • edited

codecov-io commented Nov 28, 2017 • edited

Codecov Report

houndci-bot Dec 5, 2017

Choose a reason for hiding this comment

mjarvis commented Dec 5, 2017

DivineDominion Dec 5, 2017 • edited

Choose a reason for hiding this comment

DivineDominion Dec 5, 2017 • edited

Choose a reason for hiding this comment

mjarvis Dec 5, 2017

Choose a reason for hiding this comment

DivineDominion Dec 5, 2017

Choose a reason for hiding this comment

DivineDominion Dec 5, 2017

Choose a reason for hiding this comment

DivineDominion commented Dec 5, 2017

mjarvis commented Dec 5, 2017

houndci-bot Dec 5, 2017

Choose a reason for hiding this comment

houndci-bot Dec 5, 2017

Choose a reason for hiding this comment

mjarvis commented Dec 5, 2017

DivineDominion commented Dec 5, 2017

DivineDominion commented Dec 6, 2017

DivineDominion commented Dec 6, 2017

mjarvis commented Nov 27, 2017 •

edited

codecov-io commented Nov 28, 2017 •

edited

DivineDominion Dec 5, 2017 •

edited

DivineDominion Dec 5, 2017 •

edited