wwilsman / redux-deep-diff

Higher order reducer to deep diff redux states
MIT License
23 stars 4 forks source link

Usage with freezing libraries #11

Closed jeandat closed 4 years ago

jeandat commented 4 years ago

This might be a stupid question but how do you make this library works with tools like immer or ngrx-store-freeze that freeze the state after reducers have been called. Cause redux-deep-diff is a wrapper around real reducers. Thus when it tries to instrument the state, everything is frozen.

wwilsman commented 4 years ago

Hi Jean! Thanks for opening an issue!

Could you give me an example of how you would set up your store with one of these tools? redux-deep-diff doesn't have to wrap the whole state or other reducers, it can be used on leafs of the state as well. Perhaps you can change which reducer is the topmost reducer?

I'm also not sure how redux-deep-diff would affect those libraries or vice versa since redux-deep-diff does not mutate any state

jeandat commented 4 years ago

It looks like this:

const reportReducer = combineReducers({
  ui: combineReducers<any, any>({
    index: fromIndex.reducer,
    report: fromReportUi.reducer
  }),
  data: combineReducers<any, any>({
    report: fromEntity.reducer,
    item: fromItem.reducer,
    page: fromPage.reducer,
    'template-variable': fromParameter.reducer
  })
});

const reducerWithDiff = diff(reportReducer, {
  key: 'history',
  limit: 50,
  prefilter: (path: string[], key: string) => {
    return key === 'ui' && path.length === 0;
  },
  undoType: ReportActionTypes.Undo,
  redoType: ReportActionTypes.Redo,
  jumpType: ReportActionTypes.Jump,
  clearType: ReportActionTypes.ClearHistory,
  ignoreInit: false
});

// getReducers is provided to ngrx so it can select the right reducer depending on the message sent to the store
function getReducers(): ActionReducer<ModuleReportState> {
  return (state, action) => {
    if (...) {
      return reducerWithDiff(state, action);
    }
    return reportReducer(state, action);
  };
}
wwilsman commented 4 years ago

This looks like it should work, are you getting an error, or is it just not recording anything within the history segment? I see you're ignoring diffs from ui with that prefilter option, you could also only apply the diff function to data in your combineReducers options rather than wrap it around the entire state.

jeandat commented 4 years ago

You're right there is an optimization to be made with data. prefilter is not needed. Nonetheless this doesn't change the problem at heart.

I got that error in the console after a Jump message:

arrayRemove@http://localhost:8080/vendor.js:236688:1608
revertArrayChange@http://localhost:8080/vendor.js:236688:6068
revertChange@http://localhost:8080/vendor.js:236688:6387
doChanges@http://localhost:8080/vendor.js:236690:266
doDiffs@http://localhost:8080/vendor.js:236690:469
diff/<@http://localhost:8080/vendor.js:236696:2788

Also redux devtools browser extension shows this:

TypeError: "length" is read-only

My understanding is that redux-deep-diff try to do its job by instrumenting the history object which was frozen previously by the underlying reducer and then fail.

I can workaround the issue by disabling freezing totally but that's too bad cause it is a nice protection in development that I would like to keep.

wwilsman commented 4 years ago

Okay, I've done quite a bit of testing and research and I'm pretty sure I see whats going on here.

This library does indeed work with frozen objects. However, it does not work when those objects are not plain objects. Here's the test:

https://runkit.com/wwilsman/5e7242405593bc0013beff18

The notes there explain it. When the state contains an object that is not a POJO, a reference is returned when cloning. Since revertChanges mutates the target, the clone contains a reference to a frozen object and throws that error.

Here is the documented limitation of clone-deep.

While we could pass a second option to clone-deep to make it clone instances, I don't think that's the recommended route. Redux's own guides recommend ensuring that your state is serializable as JSON, which typically means using JSON specific values (plain objects, arrays, strings, booleans, and numbers only).

I would recommend making sure your state does not contain custom instances and only use POJOs that satisfy the isPlainObject check made during cloning.

jeandat commented 4 years ago

Unfortunately as far as I can tell I'm only using simple POJO. I did check what was returned by reportReducer to be sure with isPlainObject. And it returns true.

wwilsman commented 4 years ago

You'll have to make sure every object within your state tree returns true for isPlainObject since that utility is not recursive.