search

mattkrick / redux-optimistic-ui

a reducer enhancer to enable type-agnostic optimistic updates
MIT License
693 stars 36 forks source link

If state is undefined, populate to default state #2

Closed RaitoBezarius closed 8 years ago

RaitoBezarius commented 8 years ago

This ensures that the enhanced reducer will be sanity-proof for Redux tests.

mattkrick commented 8 years ago

Thanks for this, I still can't visualize what is going wrong. Could you write a failing test case or point me towards a reproduction so I could see?

RaitoBezarius commented 8 years ago

@mattkrick I think that there is no need: https://github.com/rackt/redux/blob/master/src/combineReducers.js#L52

mattkrick commented 8 years ago

No need for the PR or for the test? Not every case will use the redux combineReducers, some use an immutable version, and some do without. I just can't think of a case where isReady would be true & the state would be undefined.

RaitoBezarius commented 8 years ago

@mattkrick Sorry for the brief answer. https://github.com/rackt/redux/blob/master/src/combineReducers.js#L52 On this line, you can see that all reducers will be called with undefined state for the initialization.

And after this, they will be called a second time: https://github.com/rackt/redux/blob/master/src/combineReducers.js#L64

With again undefined state.

While I understand that not everyone won't use the combineReducers helper.

This is just a sanity test: https://github.com/rackt/redux/blob/master/src/combineReducers.js#L49 from Redux to ensure that the reducer is "spec-proof" (in my opinion).

So what I mean is that here you have two calls with undefined state, a simple isReady won't be sufficient, I suggest just to return initialState in case of undefined state, what do you think?

mattkrick commented 8 years ago

is the current implementation throwing that sanity check error for you? I still can't visualize a scenario where this would cause an error. It's stated that the initial state can't be undefined, and this PR would it make so no future state could be undefined. is that the goal?

RaitoBezarius commented 8 years ago

@mattkrick Yes, the current implementation is throwing that sanity check error.

This PR would make sure that you return initialState if you get a undefined state in input.

But, in the current impl, you do return undefined state in case if isReady === true and I pass undefined state as input.

mattkrick commented 8 years ago

ahhh! gotcha. yeah, i guess doing it this way would make it impossible to have a nextState be undefined, but I can't think of a single scenario where that would be a hard requirement. Thanks!

RaitoBezarius commented 8 years ago

@mattkrick No problem! It depends on your implementation, but as Redux "spec" says "you will return a valid state no matter what happens", it is always better, I imagine!