nilsmehlhorn / ngrx-wieder

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

History not filling #58

Closed Taldorr closed 2 years ago

Taldorr commented 2 years ago

Hi,

I'm working on a somewhat legacy project I managed to update to Angular 11 and introduced immer + wieder.

Unfortunately my code does not work as expected.

This is my reducer file (shortened)

export interface IDashboardState extends UndoRedoState {
  dashboards: DashboardModel[]
  currentDashboardId: string | undefined
}

export const initialState: IDashboardState = {
  dashboards: [],
  currentDashboardId: undefined,
  ...initialUndoRedoState
}

const undoableActions = [
  DashboardActions.updateWidget,
  DashboardActions.addWidgetToDashboard,
  DashboardActions.removeWidgetFromDashboard
]

const { createUndoRedoReducer } = undoRedo({ allowedActionTypes: undoableActions.map(a => a.type) })

export const dashboardReducer = createUndoRedoReducer(
  initialState,
  produceOn(DashboardActions.addWidgetToDashboard, (state, action) => {
    const dashboardIdx = state.dashboards.findIndex(d => d.id === action.dashboardId)
    if (dashboardIdx >= 0) {
      state.dashboards[dashboardIdx].widgets.push(action.widget)
    }
    return state
  }),
)

One thing that confuses me, is that I have to return the state, even though I use "produceOn":

TS2345: Argument of type '(state: WritableDraft<IDashboardState>, action: { dashboards: DashboardModel[]; } & TypedAction<"[ProjectEffects] Set Dashboards"> & { ...; }) => void' is not assignable to parameter of type 'ProduceOnReducer<void, [ActionCreator<"[ProjectEffects] Set Dashboards", (props: { dashboards: DashboardModel[]; }) => { dashboards: DashboardModel[]; } & TypedAction<"[ProjectEffects] Set Dashboards">>], void>'.   Types of parameters 'state' and 'state' are incompatible.     Type 'void' is not assignable to type 'WritableDraft<IDashboardState>'.

The other, more problematic thing is that I can't undo anything. I debugged the history within the state and found out, that it is an empty object even after dispatching multiple allowed actions.

These are the versions:

"@angular/core": "11.2.14"
"immer": "9.0.12",
"ngrx-immer": "1.0.3",
"ngrx-wieder": "6.0.2",

What might be the problem and how could I fix it?

Best regards

nilsmehlhorn commented 2 years ago

Hi @Taldorr ,

I'm not sure why the history would be empty. Would you be able to provide a small reproduction (e.g. GitHub repository or StackBlitz)?

Type 'void' is not assignable to type 'WritableDraft'

Again, not sure. produceOn is supposed to accepts an action reducer that returns void. Here's the example for Angular 11 and ngrx-wieder 6, specifically the line that does that:

https://github.com/nilsmehlhorn/ngrx-wieder-example/blob/d592f12e115271e774a3c92aff383109b6657449/src/app/on.reducer.ts#L39

Maybe you can also compare your setup with the example app for those versions in general:

https://github.com/nilsmehlhorn/ngrx-wieder-example/tree/d592f12e115271e774a3c92aff383109b6657449

Taldorr commented 2 years ago

Hi @nilsmehlhorn,

Thanks for your time. I reconstructed a simplified StackBlitz: https://stackblitz.com/edit/angular-ivy-fe6vqt

I don't know why, but returning the state in the reducer is not needed there, the state draft is not updateable with standard runtime checks though. For simplicity I disabled the according runtime checks (see app.module.ts L17) .

Without any configuration of undoRedo() (see dashboard.reducer.ts L27) the history object is filled. However, after adding an Widget with the first button, I'd expect the UNDO-Button to remove this widget, instead it kind of deletes the whole state.

When setting specific allowedActionTypes to the undoRedo() the history object will stay empty

nilsmehlhorn commented 2 years ago

First off, the example is using different versions than you initially specified:

"@angular/core": "12.2.16"
"immer": "9.0.12",
"ngrx-wieder": "8.0.2",

Also, it's not using timdeschryver/ngrx-immer of which I'm not sure whether it's compatible with ngrx-wieder anyway though.

Further, it seems like you're storing classes in your state (e.g. DashboardModel) which isn't a good idea because

a. NgRx prefers serializable and immutable state b. Classes aren't drafted by immer.js per default

Technically you could work around this by turning off runtime checks and marking the classes as immerable, but I'd strongly advise against this (had the same problem in a project and this stopped working at some NgRx/immer version). Instead replace your classes with plain JavaScript objects (typed via interfaces or type declarations) - which shouldn't be too hard for e.g. DashboardModel since it's only a data structure.

Taldorr commented 2 years ago

Thank you for this explanation. I tried using interfaces and it worked like a charm. As you mentioned, immerable does not work anymore. I appreciate your time and effort and will close this issue.