Closed dcurletti closed 5 years ago
Hi @dcurletti, thanks very much for opening this issue. You're totally right that the example is broken. I have some somewhat rambling thoughts about how to handle it.
I think this is a docs problem, not a library problem. The only thing wrong with the upcasting reducer example is that the function reducer
in the example no longer satisfies the Reducer
interface after Redux changed it at some point (much more detail below). Simply changing the documentation so that the function reducer()
has an initial state will fix the docs, and this isn't a problem in practice because the incorrect example will fail typechecking when initializing the store or calling combineReducers
with it.
I'm not sure that giving upcastingReducer
an initial state is the right fix. It's really hard to make typings with regard to initial state that make everyone happy. Here's a short history of the issues involved:
Originally, the official Redux typings declared a reducer as (State, Action) => State
, without allowing for the possibility of passing undefined state, and this library followed suit with the same definition. This was a technically incorrect definition, since reducers need to handle undefined as being initial state in the standard Redux usage, so users could define functions satisfying the Reducer
type which could not actually be used with Redux. However, this definition has the advantage for this library that both reducerWithInitialState(initialState)
and reducerWithoutInitialState()
could return the type Reducer
and soundly satisfy its type definition. See the former definition here.
At some point, the official Redux typings changed to define a reducer as (State?, Action) => State
, as in the current definition as of today. This is more correct, but unfortunately makes reducerWithoutInitialState
awkward for this library, as it can no longer honestly say it is returning a function meeting the Reducer
interface. This gives this library a choice:
reducerWithoutInitialState
returns a Reducer
. This is unsound, because if passed to a store which does not have an initial state then it will fail.undefined
state. This makes it type-incompatible with Redux, which defeats the point of this library.You might ask, why have reducerWithoutInitialState()
at all in that case? It's actually a pretty useful way to structure some apps. In many applications, the initial state depends on some environmental factors, such as cookies or local settings, or perhaps it doesn't make sense to create the store at all until certain data is retrieved from the server. In this case, it doesn't make sense for "dumb" reducers to produce the same initial state every time- it makes more sense to programatically construct an initial state and pass it to your store as you create the store. The Redux authors have argued that putting the initial state in the reducers is better because it ensures that the shape of the state matches the shape of the reducers, but one might argue that we don't have to worry about that, because we use TypeScript!
Because of this, despite the typings around reducerWithoutInitialState
being unsound, the function is still pragmatically useful and so will remain in the library.
Back to the original point: as Reducer
s are now required to accept undefined
as their first argument, the example written in the README will not typecheck and so this isn't a problem with the library but with the docs. I'll change the docs and consider this fixed, unless you have an argument for why making upcastingReducer
take an initial state is the right solution instead.
Will leave a longer response as to why tomorrow, but I agree that upcastingReducer shouldn't take an initial state.
Yea, sorry, I should've been more explicit that this was only a docs problem.
The example in the Readme of the
upcastingReducer
doesn't show that the final "reducer" should have an initial state passed to it in order for it not to error.Reason: someone copy and pasting that example and exporting the final function as the reducer will get an error when they try to run the code or test it.
I can submit a PR for this later today, but the fix is just creating an
INITIAL_STATE
variable that is given as the default to the final "reducer".