Store Update - merges current and previous state values. #661

field123 · 2021-04-25T12:34:19Z

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 text was updated successfully, but these errors were encountered:

NetanelBasal · 2021-04-26T05:29:17Z

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

field123 · 2021-04-26T11:31:09Z

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?

NetanelBasal · 2021-04-26T11:35:13Z

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

field123 · 2021-04-26T12:21:43Z

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?

NetanelBasal · 2021-04-26T12:23:43Z

Ok, let's go with option two

field123 · 2021-04-26T12:26:57Z

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

Created a public method on the akita store that will completely replace the current state with the new provided state and not just update the value. Closes salesforce#661

Renamed the new set method to overwrite in an effort to avoid conflicts with the entityStore method set. Closes salesforce#663 PR salesforce#661

Removed redundant local functions in favour of inlining. Simplified the documentation example for overwrite() Closes salesforce#661 PR salesforce#663

clark-stevenson · 2022-09-01T10:37:18Z

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!

field123 linked a pull request Apr 26, 2021 that will close this issue

feat(store): add set method to akita store #663

Open

13 tasks

field123 pushed a commit to field123/akita that referenced this issue Apr 27, 2021

fix(store): renamed set method to overwrite

f08f610

Renamed the new set method to overwrite in an effort to avoid conflicts with the entityStore method set. Closes salesforce#663 PR salesforce#661

field123 pushed a commit to field123/akita that referenced this issue Apr 28, 2021

fix(store): removed redundant local functions

f1d8eb0

Removed redundant local functions in favour of inlining. Simplified the documentation example for overwrite() Closes salesforce#661 PR salesforce#663

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store Update - merges current and previous state values. #661

Store Update - merges current and previous state values. #661

field123 commented Apr 25, 2021

NetanelBasal commented Apr 26, 2021

field123 commented Apr 26, 2021

NetanelBasal commented Apr 26, 2021

field123 commented Apr 26, 2021

NetanelBasal commented Apr 26, 2021 •

edited

field123 commented Apr 26, 2021 •

edited

clark-stevenson commented Sep 1, 2022 •

edited

Store Update - merges current and previous state values. #661

Store Update - merges current and previous state values. #661

Comments

field123 commented Apr 25, 2021

I'm submitting a...

Current behavior

Expected behavior

Minimal reproduction of the problem with instructions

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

Environment

NetanelBasal commented Apr 26, 2021

field123 commented Apr 26, 2021

NetanelBasal commented Apr 26, 2021

field123 commented Apr 26, 2021

NetanelBasal commented Apr 26, 2021 • edited

field123 commented Apr 26, 2021 • edited

clark-stevenson commented Sep 1, 2022 • edited

NetanelBasal commented Apr 26, 2021 •

edited

field123 commented Apr 26, 2021 •

edited

clark-stevenson commented Sep 1, 2022 •

edited