Reswift: make Store.state setter private

[DivineDominion]

Found this little gem in the Store code today:

 // TODO: Setter should not be public; need way for store enhancers to modify appState anyway
/*private (set)*/ public var state: State!

Making the setter private doesn't break any tests.

@Ben-G added this back in commit 0effbd7 (Jan 2016) -- What kind of enhancers did you have in mind?

[mjarvis]

@Ben-G Been a while. Do you recall what kind of enhancers you had in mind here?

[Ben-G]

Hey folks! Sorry, missed the notification here. The only current user of this API would be ReSwift-Recorder. It needs the ability to rewind/forward to specific states without dispatching actions.
Since I basically have done no work on the recorder since the original prototype I think we can go ahead and fix this TODO. If I or anyone else puts effort into building a non-prototype recorder we can figure out a way to extend the store API to accommodate that.

[DivineDominion]

I'm not against breaking the API if it's all just for _ReSwift-Recorder_, but I think some people are using it in production 🤔

Was there ever any reason not to make the recorder come with a state replacement reducer and dispatch an action that triggers this change? If not, maybe we can push that first, then make the state private again.

[dani-mp]

Using ReSwift-Recorder or the setter?

[DivineDominion]

Using the Recorder. Breaking the setter wouldn't be too much of a problem for a major version release, but not offering a working Recorder might be a downer for some folks.

[dani-mp]

I understand. To be honest I've never seen the code of the Recorder library. As far as I know, a recorder is usually implemented as a higher order reducer, which means that setting the state directly shouldn't be necessary, but if it's implemented like that, it's true that it'd need a rewrite.

More importantly, being able to set the state directly in ReSwift could be confusing for people that are new to the library, because it's an anti-pattern and, depending on the implementation of the library, it could cause bugs on the client side, so I'd remove it as soon as possible.

[mokagio]

More importantly, being able to set the state directly in ReSwift could be confusing for people that are new to the library, because it's an anti-pattern and, depending on the implementation of the library, it could cause bugs on the client side, so I'd remove it as soon as possible.

The ability to read and write the StateType from store.state has lead to all sorts of misuses of ReSwift in our codebase 😞 .

[mokagio]

Making state private in Store.swift does break some tests in the current HEAD of master (40179c2ed056b17fee37b4bf9fa87b95d935df48).

E.g.:

An idea on how to work around that could be to have a middleware for the purpose of testing that records all the actions it receives. Using a reducer would send a bad example, as it wouldn't be pure anymore.

I'll see if I can play around with this and submit a PR.

[DivineDominion]

Maybe internal private(set) var state: StateType so tests and library internals can read it? :)

[Dimillian]

Wow, I'm accessing store.state in a lot of UI. I subscribe only to get notification for some changes sometime, but I don't necessary copy the state locally. I trigger a refresh function. Would break a shitload of stuff in my implementation. Good direction tho, I agree with the change in term of architecture.

But it's not an anti pattern to access the state from the store in some cases.

[dani-mp]

Yeah, the anti-pattern would be just to set the state directly, but the store should always provide a way to query its current state, either via a method or a property.

[DivineDominion]

@Dimillian would love to see an article that shows how you do that (so others can learn what works -- and I can maybe understand why you do that :))

[Dimillian]

@DivineDominion I should write one, and then live with the fear to be teared appart by the Redux community :p But the truth is that the need and final architecture is not the same in a native mobile app than it is in a react web app.

If your states are all struct based, there is absolutely no problem accessing store.state, as you'll always access a safe final copy anyway. The only problem could be that you access a future version or a different version that something you would get from your newState subscribed function. Is it a problem? Depend of your app. Is it anti pattens? A bit maybe.

import UIKit
import UI
import ReSwift

class LabeledTableViewCell: UITableViewCell, {

    var user: ObjectId!
    var shelve: ShelveUser! {
        didSet {
            store.subscribe(self) {
                $0.select{ $0.usersState }.skipRepeats({ [weak self] (oldState, newState) -> Bool in
                    guard let weakself = self else {
                        return true
                    }
                    return oldState.users[weakself.user]?.userShelves[weakself.shelve] ==
                        newState.users[weakself.user]?.userShelves[weakself.shelve] &&
                        oldState.users[weakself.user]?.userShelvesCount[weakself.shelve] ==
                        newState.users[weakself.user]?.userShelvesCount[weakself.shelve]
                })
            }
            refresh()
        }
    }

    @IBOutlet var titleLabel: CellTitleLabel!
    @IBOutlet var detailLabel: CellDetailLabel!

    override func prepareForReuse() {
        super.prepareForReuse()

        store.unsubscribe(self)
    }

    func refresh() {
        if let user = store.state.usersState.users[user], let count = user.userShelvesCount[shelve] {
            detailLabel.text = "\(count)"
        }
        titleLabel.text = shelve.title()
    }
}

extension LabeledTableViewCell: StoreSubscriber {
    func newState(state: UsersState) {
        DispatchQueue.main.async {
            self.refresh()
        }
    }
}

And let's imagine that refresh function is much more complex, and need to access stuff out of your subscription from your store other states.

[Dimillian]

Annnnnd, I see that this discussion is about the setter specifically, of course I'm not setting anything outside of the reducers. So I guess I'm a bit ouf of topic.

[DivineDominion]

Closed via #354