redux-saga-test-plan icon indicating copy to clipboard operation
redux-saga-test-plan copied to clipboard

using withState and withReducer together

Open epikhighs opened this issue 6 years ago • 10 comments

After some experimentation, it appears we can chain withState after withReducer to set initial state. If I chain withState before withReducer, then that initial state is discarded. However, I'm wondering if this behavior is a feature we should be using or we really should only use withReducer and pass in the initial state? If we can use both withState and withReducer, it would be nice if ordering did not matter similar to how the .put() works. .put() just expects a put() will occur at some point in the saga.

I find myself naturally gravitating to a syntax where I use withState and withReducer b/c it explicitly tells me what's going on (that is: I'm using these reducers and this state). Whereas, passing two args into withReducer is less explicit.


      expectSaga(someSaga)
        /// does not work!  this initial state will be discarded after withReducer
        // would be nice if ordering did not matter
        .withState({
          reducer1: {},
        })
        .withReducer(combineReducers({
          reducer1,
        }))
        .run();

      // declarative and and clear what's going on - imo
      expectSaga(someSaga)
        .withReducer(combineReducers({
          reducer1,
        }))
        .withState({
          reducer1: {},
        })
        .run();

      // a little vague - imo
      expectSaga(someSaga)
        .withReducer(combineReducers({
          reducer1,
        }), {
          reducer1: {},
        })
        .run();

Any thoughts?

epikhighs avatar Feb 08 '18 01:02 epikhighs

Hi, @epikhighs.

I initially introduced withState before withReducer, so that's part of the reason why they don't play nice together. I'm not opposed to changing the behavior, but I still think having the implicit initial state from a reducer would be useful. Maybe if initial state wasn't set withwithState, then we could use the implicit initial state from the reducer.

I'm happy to accept a PR that changes the behavior to let the two methods cooperate more nicely.

jfairbank avatar Feb 17 '18 22:02 jfairbank

Thanks for the background info @jfairbank . I'm a little busy at the moment, but if I find the time, I'll setup a PR. Otherwise, any else who's interested can contribute as well.

epikhighs avatar Feb 21 '18 08:02 epikhighs

This works, try to establish the state of the reducer after adding the reducer? .withReducer(reducer) .withState(state) @jfairbank

tastafur avatar May 24 '18 15:05 tastafur

My intuition was that setting withState first would ensure that the reducer had a state to work with, and wouldn't fall back on its default state, in common with the fundamentals of redux itself.

The most confusing thing about this behaviour was in some sense it was the complete reverse of redux behaviour: configuring a reducer even when a state is already set forces the reducer's default value in place of the existing state.

Here was my minimal test case which finally narrowed down the issue. The second test in the suite is the one which fails, counterintuitively.

const { select } = require("redux-saga/effects")
const { expectSaga } = require("redux-saga-test-plan")

function* selectMapSaga(key) {
  return yield select(state => state.maps[key])
}

const noopReducer = (state = [], action) => state

describe("Test withState and withReducer order", () => {

  it("withState afterwards controls state as expected", () => {
    const testMapKey = "test-schema-key"
    return expectSaga(selectMapSaga, testMapKey)
      .withReducer(noopReducer)
      .withState({ maps: {} })
      .dispatch({})
      .run()
  })

  it("withReducer afterwards overrides state with its default", () => {
    const testType = "test-focus-type"

    return expectSaga(selectMapSaga, testType)
      .withState({ maps: {} })
      .withReducer(noopReducer)
      .dispatch({})
      .run()
  })

})

cefn avatar Oct 13 '19 11:10 cefn

As a short-term measure, recording in the documentation that setting a state via withState and via withReducer are incompatible with each other would have saved me (and perhaps others) a lot of work, even in advance of changing the behaviour.

cefn avatar Oct 13 '19 12:10 cefn

It is pretty clear from documentation that withState and withReducer shouldn't be used together. They are intended for two separate use cases - withState for static state and withReducer for state that might change. So there is nothing to fix in the library code, maybe only add NOTE or WARNING in the documentation that their joint presence in single expect assertion is an invalid API usage

vatosarmat avatar Oct 13 '19 19:10 vatosarmat

@vatosarmat can you point to where in the documentation this is clear?

The scenario you describe (state that might change) could easily be understood (by non experts who have not read this thread) to be achieved by the following

      .withState({ myName: myValue }) //define the initial state
      .withReducer((state = {}, action) => state) //define the reducer

A Redux-aware developer would be surprised to see their clearly initialised state replaced with the reducer function's javascript default, given the conventional purpose of the default value in Redux (to only use it if there ISN'T already a state defined).

If the use of withState and withReducer are directly incompatible with each other as you say (e.g. compositions including both are invalid, have unpredictable effects, or one will be 'unreachable code') then there could well be something to fix in the library as well as the documentation, since using them jointly is an error. At the very least an error of intent, even if you are lucky and it doesn't cause an actual error.

cefn avatar Oct 18 '19 10:10 cefn

@cefn I understand your reasoning, my intuition was same as yours and I spent some time trying to understand why my tests are falling. But I tend to think it's my mistake cause I didn't read the documentation. this page has 2 subsections: Static State via withState and Dynamic State via withReducer and for me it looks like kind of clear distinction between 2 use cases.

Also, a Redux-aware developer knows about createStore(reducer, [initialState]), and withReducer has similar parameters: withReducer<S>(newReducer: Reducer<S>, initialState?: S).

They are not incompatible, they just not intended to be used together because the 2nd just overwrites the 1st. You can do withState(foo).withReducer(reducer, bar).withState(johny) that's not an error at all, but that doesn't have any sense because you just overwrite variables storeState and reducer defined inside expectSaga function.

Maybe expectSaga should do some check and throw error if with[Reducer|State] called more than once?

vatosarmat avatar Oct 18 '19 12:10 vatosarmat

Thanks for clarifying @vatosarmat

Maybe expectSaga should do some check and throw error if with[Reducer|State] called more than once?

Yes, that's the kind of safety net I'm imagining. If there's anything other than a single call to either withReducer or withState then a meaningful error could be thrown to indicate it definitely isn't going to do what you think.

cefn avatar Oct 18 '19 17:10 cefn

@jp928 what do you think? Presense of more than one with[Reducer|State] in single expectSaga looks pointless and might be confusing for some users. Maybe it should be considered erroneous and throw and exception?

vatosarmat avatar Oct 19 '19 03:10 vatosarmat