Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does not play well with reducers created with redux-toolkit's createSlice #272

Open
posita opened this issue Jun 24, 2020 · 1 comment
Open

Comments

@posita
Copy link

posita commented Jun 24, 2020

This might be a duplicate of #268, but I think this weighs in favor of treating #268 as a real bug (instead of as-designed) and actually fixing it. The short version is as follows. I created a slice with createSlice from reduxjs/redux-toolkit with an incrementNextId action:

export interface SliceState {
  next_id: number;
}

const INITIAL_STATE: SliceState = {
  next_id: 1,
}

export const slice = createSlice({
  name: "slice",
  initialState: INITIAL_STATE,
  reducers: {
    incrementNextId: { /* ... */ }
  },
})

export const {
  incrementNextId,
} = slice.actions

export const slice_reducer = slice.reducer
export const undoable_slice_reducer = undoable(slice_reducer)

Here's the rub. Using the plain reducer works just like it should according to Redux's instructions on how to test reducers:

  describe("plain increment", () => {
    // this passes
    it("should increment next_id", () => {
      var state = slice.slice_reducer(undefined, slice.incrementNextId({}))
      expect(state.next_id).toEqual(2)
    })
  })

Using the wrapped reducer does not with a nearly identical test (the only difference is which reducer is used and how we inspect state):

  describe("undoable increment", () => {
    // this fails
    it("should increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))
      expect(state.present.next_id).toEqual(2)  // nope, the value is actually 1
    })
  })

If I modify the second test as follows, it passes, which doesn't seem right:

  describe("undoable increment", () => {
    // this works, but it shouldn't
    it("should increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))  // ignored?
      state = slice.undoable_slice_reducer(state, slice.incrementNextId({}))  // this time, with feeling!
      expect(state.present.next_id).toEqual(2)  // works now?
    })
  })

Here's where things get totally off the rails. This fails both tests:

  describe("undoable increment", () => {
    // this fails
    it("should increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))
      expect(state.present.next_id).toEqual(2)  // nope, the value is actually 1
    })

    // this fails, too
    it("should also increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))
      expect(state.present.next_id).toEqual(2)  // nope, the value is actually 1
    })
  })

If I make the above modification to the first test, both tests now pass:

  describe("undoable increment", () => {
    // this works, but it shouldn't
    it("should increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))  // ignored?
      state = slice.undoable_slice_reducer(state, slice.incrementNextId({}))  // this time, with feeling!
      expect(state.present.next_id).toEqual(2)  // works now?
    })

    // this now…works?
    it("should also increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))
      expect(state.present.next_id).toEqual(2)  // now this passes; WTF?!
    })
  })

If I move that approach to the second test, only it passes:

  describe("undoable increment", () => {
    // this fails
    it("should increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))
      expect(state.present.next_id).toEqual(2)  // nope, the value is actually 1
    })

    // this now…works?
    it("should also increment next_id", () => {
      var state = slice.undoable_slice_reducer(undefined, slice.incrementNextId({}))  // ignored?
      state = slice.undoable_slice_reducer(state, slice.incrementNextId({}))  // this time, with feeling!
      expect(state.present.next_id).toEqual(2)  // works now?
    })
  })

Complete source code is available via posita/redux-toolkit-undo-clash.

@posita
Copy link
Author

posita commented Jun 24, 2020

One can work around this by adding the following to one's test case, but that just helps illustrate how weird this behavior is:

    beforeEach(() => {
      var state = slice.undoable_slice_reducer(undefined, {} as AnyAction)  // first no-op
      slice.undoable_slice_reducer(state, {} as AnyAction)  // second no-op
    })

Both lines are necessary. If you omit the second, you'll still get failures.

@posita posita changed the title redux-undo does not play well with reducers created with redux-toolkit's createSlice Does not play well with reducers created with redux-toolkit's createSlice Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant