Make state an internal property of Store #354
Conversation
Having the State value accessible in the Store can lead to misuses of ReSwift, where state is accessed by reading it from the store rather than subscribing to it.
This way only internals of ReSwift can access it, e.g. for testing purposes, while consumers of the framework can't. This should promoted a better usage of ReSwift, where the only way to read the State and its changes is to subscribe to the Store.
mokagio
force-pushed
the
private-store-state
branch
from
7d1b6a7
to
7f3364f
Compare
Codecov Report
@@ Coverage Diff @@
## master #354 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 5 5
Lines 176 176
=======================================
Hits 175 175
Misses 1 1
Continue to review full report at Codecov.
|
ReSwift/CoreTypes/StoreType.swift
Outdated
| @@ -18,9 +18,6 @@ public protocol StoreType: DispatchingStoreType { | |||
|
|
|||
| associatedtype State: StateType | |||
|
|
|||
| /// The current state stored in the store. | |||
| var state: State! { get } | |||
There was a problem hiding this comment.
I don't think this change should be included in this PR. There are valid reasons for making state unreadable, but there are also valid reasons for reading state outside of a subscriber (IE one-time reads). Whether or not it stays I think this should be discussed & dealt with separately from the change to removing write access.
There was a problem hiding this comment.
I agree with that. Making the state totally private from the store could be a source of a lot of problem in some architectures.
There was a problem hiding this comment.
The original Redux implementation in JavaScript has a getState() method to query the state of the store at any time, so that should be enough, IMO!
There was a problem hiding this comment.
Instead of Java-style getters like getState(), making the property puplicly readable via public private(set) could work.
There was a problem hiding this comment.
Would be perfect. Setter can and should be private anyway.
There was a problem hiding this comment.
Yeah, they probably do it because of JavaScript, but in Swift we could just use the read-only variable. It's the same as in the middleware, you get both the dispatch function and getState.
|
Hey folks. Thanks for all the great comments. Unfortunately I haven't had and still don't have the bandwidth to follow this up 😞 . Feel free to pick up the branch if you want, or close this PR and start a new one. |
This reverts commit 6ba7e24.
[ci skip]
The getter must be public.
This PR removes the
Stateproperty from theStoreTypeprotocol, and makes itprivate(set) internalin theStoreimplementation.The aim of this change is to promoted a better usage of ReSwift, preventing consumers from falling into the anti-pattern of reading the
Statedirectly from theStore, rather than conforming toStoreSubscriber. See the discussion on #264.