nilsmehlhorn / ngrx-wieder

Lightweight undo-redo for Angular with NgRx & immer.js
https://nils-mehlhorn.de/posts/angular-undo-redo-ngrx-redux
MIT License
125 stars 10 forks source link

[Immer] An immer producer returned a new value *and* modified its draft #81

Closed julianpoemp closed 1 year ago

julianpoemp commented 1 year ago

Scenario

In a complex application I implement some type of spreadsheet. On initialization I create the table and add a row, something like that:

newState = {
        ...newState,
        table: {
          ...newState.table,
          rows: [newRow]
        }
      };

Where "newRow" was created before. If I keep this code block, an error occurs (see below). If I remove it, the error disappears. After a lot of testing I'm out of new ideas. Could this be a bug in ngrx-wieder?

Further information

Error message

ERROR Error: [Immer] An immer producer returned a new value *and* modified its draft. Either return a new value *or* modify the draft.
    at n (immer.esm.js:1:216)
    at P (immer.esm.js:1:2508)
    at produce (immer.esm.js:1:16338)
    at reducer (ngrx-wieder.mjs:52:27)
    at ngrx-wieder.mjs:196:35
    at ngrx-store.mjs:374:20
    at combination (ngrx-store.mjs:329:37)
    at ngrx-store.mjs:994:27
    at ngrx-store.mjs:362:20
    at computeNextEntry (ngrx-store-devtools.mjs:457:21)

ngrx-wieder/fesm2020/ngrx-wieder.mjs:52:27

const create = (initialState, ons, config, segmenter) => {
    const map = {};
    for (const on of ons) {
        for (const type of on.types) {
            if (map[type]) {
                const existingReducer = map[type];
                map[type] = (state, action) => on.reducer(existingReducer(state, action), action);
            }
            else {
                map[type] = on.reducer;
            }
        }
    }
    const reducer = ((state = initialState, action, listener) => {
        const r = map[action.type];
        if (r) {
            return produce(state, (draft) => r(draft, action), listener);   // <------------------- ERROR HERE?
        }
        return state;
    });
    return wrap(reducer, config, segmenter);
};

I searched for this error message on Google and found this: https://stackoverflow.com/questions/60806105/error-an-immer-producer-returned-a-new-value-and-modified-its-draft-either-r

Does it have something to do with the definition of anonymous functions by any chance?

System

nilsmehlhorn commented 1 year ago

Hi @julianpoemp, can you show more of your reducer code please and optimally create a reproduction?

Could this be a bug in ngrx-wieder?

Did you try removing ngrx-wieder and still receive the error?

Does it have something to do with the definition of anonymous functions by any chance?

Hm, at a glance, maybe. But I'd be wondering why this problem would only show up now. However, if that's the problem it should be pretty easy to reproduce :)

julianpoemp commented 1 year ago

Hi @nilsmehlhorn, thank you for your fast response! I was able to create a minimal working example that reproduces this issue. You can find it here: https://github.com/julianpoemp/ngrx-wieder-immer-bug The README contains further information. Thank you very much for looking into this!

nilsmehlhorn commented 1 year ago

I've looked at your app and interestingly enough this line seems to be causing the error (not sure if it's the root cause though): https://github.com/julianpoemp/ngrx-wieder-immer-bug/blob/0556f4fa1e3cc93b462c55681e2a3c69fdfa966e/apps/immer-draft-bug/src/app/store/table.reducer.ts#L177

Changing the value of the type property to a static string makes the error disappear for me. That's really odd as it is only some nested string value.

Overall your reducer isn't really in line with how you'd ideally use immer. You're performing immutable updates via shallow copy instead of mutating the draft - which is fine in theory but you need to be careful not to accidentally do both mutable and immutable updates (i.e. return a new object). However, I don't really see how that particular line would be doing that. Please see here for additional info: https://immerjs.github.io/immer/return

Would you mind trying to refactor at least the action reducer for TableActions.init.success to use the mutable update patterns described here to see if that helps?

Eventually, you're also welcome to provide a pull request with a test case for the issue and a corresponding fix. I don't have loads of capacity for that right now.

julianpoemp commented 1 year ago

@nilsmehlhorn thank you very much for your response, that helps me a lot! I was able to fix this as you recommended. At the moment I don't really understand why immer can't handle simple shallow copies:

      state.tools = tools;
      state.projectRoles = projectRoles;

works. But this fails:

      state = {
        ...state,
        tools, projectRoles
      }

So as far as I can understand it's not possible to use shallow copies as soon as I want to use immer. I'll have to read more about immer to understand it.

nilsmehlhorn commented 1 year ago

Glad it's resolved. However, immer should be able to handle shallow copies as mentioned on this page:

case "adduser-3":
    // OK: returning a new state. But, unnecessary complex and expensive
    return {
        userCount: draft.userCount + 1,
        users: [...draft.users, action.payload]
    }

Mixing both approaches is apparently the problem which can be hard to track down as mentioned for your case.