akita

akita copied to clipboard

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[x] Support request
[ ] Other... Please describe:

Current behavior

When calling store.update only the specified properties are being replaced.

If I have two possible states I want to represent in the Todos store

export type TodosState = TodosBasicState | TodosAltState;

interface TodosBasicState {
  _tag: "todos-basic-state";
  uniqueToBasicObject: {
    info: string;
  };
  basicInfo: string;
}

interface TodosAltState {
  _tag: "todos-alt-state";
  altInfo: string;
}

I intialize my todos state to a TodosBasicState

const initialState: TodosState = {
  _tag: "todos-basic-state",
  uniqueToBasicObject: { info: "test" },
  basicInfo: "Basic Info"
};

When I update the state using the following method I get a polluted state object that is a combination of the old state and the new values.

this.todosStore.update((state) => ({
      _tag: "todos-alt-state",
      altInfo: "This is some alt info.",
    }));

State object as json before update:

{
  "_tag": "todos-basic-state",
  "uniqueToBasicObject": {
    "info": "test"
  },
  "basicInfo": "Basic Info"
}

State object as json after update:

{
  "_tag": "todos-alt-state",
  "uniqueToBasicObject": {
    "info": "test"
  },
  "basicInfo": "Basic Info",
  "altInfo": "This is some alt info."
}

As you can see the the _tag and altInfo properties have been updated as expected but the rest of the state has not been removed from the previously.

The following line is the cause in store.ts. The current and new state are both being combined in a spread operation. I'm sure this is being done for a reason I'm ignorant of but is this side effect intentional?

https://github.com/datorama/akita/blob/721243c814b8835a0bec113a686c5a1400dc2f2a/libs/akita/src/lib/store.ts#L256

Expected behavior

I would expect that when calling the update method I get only the specified properties in the state and all old properites are dropped.

Expect this:

this.todosStore.update((state) => ({
      _tag: "todos-alt-state",
      altInfo: "This is some alt info.",
    }));

to turn state from:

{
  "_tag": "todos-basic-state",
  "uniqueToBasicObject": {
    "info": "test"
  },
  "basicInfo": "Basic Info"
}

to this:

{
  "_tag": "todos-alt-state",
  "altInfo": "This is some alt info."
}

If I wanted to keep values from the previous state I would expect to have to include them in the update callback using a spread or explicitly.

e.g.

this.todosStore.update(state => ({
      ...state,
      _tag: "todos-alt-state",
      altInfo: "This is some alt info."
    }));

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/akita-todos-app-pyf2xv?file=src/app/todos/state/todos.query.ts

Click update to perform the above update method and click reset to reset the store back to the initial method.

What is the motivation / use case for changing the behavior?

I want to be able to update the state without previous state values polluting the new state. If there is a better way to achieve this any advise is welcome?

Environment

Angular version: X.Y.Z


Browser:
- [ x] Chrome (desktop) version XX
- [ x] Chrome (Android) version XX
- [ x] Chrome (iOS) version XX
- [ x] Firefox version XX
- [ x] Safari (desktop) version XX
- [ x] Safari (iOS) version XX
- [ x] IE version XX
- [ x] Edge version XX

The update method will merge the current state with the new one. You can use the setState method to override it.

That does do exactly what I needed, Thank you.

With the method being marked as @internal :

https://github.com/datorama/akita/blob/721243c814b8835a0bec113a686c5a1400dc2f2a/libs/akita/src/lib/store.ts#L177

Would you be open to making (or accepting a pull request for) a public version of this functionality available to address the use case I've outlined above. Something along the lines of:

set(stateOrCallback: Partial<S> | UpdateStateCallback<S>) {
    isDev() && setAction('Set');

    let newState;
    const currentState = this._value();
    if (isFunction(stateOrCallback)) {
      newState = isFunction(this._producerFn) ? this._producerFn(currentState, stateOrCallback) : stateOrCallback(currentState);
    } else {
      newState = stateOrCallback;
    }

    const withHook = this.akitaPreSet(currentState, newState as S);
    const resolved = isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
    this._setState(resolved);
}

with a supporting hook:

// @internal
  akitaPreSet(_: Readonly<S>, nextState: Readonly<S>): S {
    return nextState;
}

Set may not be the best name for it maybe replace or something else?

I prefer to add options to the update method. update(object, { overwrite: true })

Something like:

update(stateOrCallback: Partial<S> | UpdateStateCallback<S>, options?: { override: boolean }) {
    isDev() && setAction('Update');

    let newState;
    const currentState = this._value();
    if (isFunction(stateOrCallback)) {
      newState = isFunction(this._producerFn) ? this._producerFn(currentState, stateOrCallback) : stateOrCallback(currentState);
    } else {
      newState = stateOrCallback;
    }

    newState = options?.override ? newState :  { ...currentState, ...newState };
    
    const withHook = this.akitaPreUpdate(currentState, newState as S);
    const resolved = isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
    this._setState(resolved);
  }

The only concern I would have with boolean flags in an options object is adding conditional complexity to a function call e.g. what if I want different hook behaviour for this do we start doing something like this:

const withHook = options?.override ? this.akitaPreSet(currentState, newState as S) : this.akitaPreUpdate(currentState,  { ...currentState, ...newState } as S);

Maybe it would be better to have clean simple method signatures and abstract away some of the same repeated code.

update(stateOrCallback: Partial<S> | UpdateStateCallback<S>): void {
    isDev() && setAction('Update');
    const withHookFn = (curr: S, newS: S) => this.akitaPreUpdate(curr,  { ...curr, ...newS } as S);
    this._setState(this.sharedProcess(stateOrCallback, this._value(), withHookFn));
  }

  overwrite(stateOrCallback: Partial<S> | UpdateStateCallback<S>): void {
    isDev() && setAction('Overwrite');
    const withHookFn = (curr: S, newS: S) => this.akitaPreOverwrite(curr,  newS as S);
    this._setState(this.sharedProcess(stateOrCallback, this._value(), withHookFn));
  }

  sharedProcess<S>(stateOrCallback: Partial<S> | UpdateStateCallback<S>, currentState: S, withHookFn: (c: S, n: S) => S): S {
    const constructNewState = (x: Partial<S> | UpdateStateCallback<S>) => {
      if (isFunction(x)) {
        return isFunction(this._producerFn) ? this._producerFn(currentState, x) : x(currentState);
      } else {
        return x;
      }
    }
    const resolveFinalState = (currentState: S, withHook: S): S => {
      return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
    }
    
    const newState = constructNewState(stateOrCallback, currentState);
    const withHook = withHookFn(currentState, newState);
    return resolveFinalState(currentState, withHook);
  }

This would keep the public api transparent and explicit, removing any ambiguity?

Ok, let's go with option two

Ok, I'll put together a pull request later today.

Hey guys!

I ran into this issue today which was unexpected. It would seem the PR became stale and given the passage of time, may no longer be relevant/required?

Would you still suggest using the internal _setState() as a work around for now?

Thanks!