Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ee3908
feat(store): add set method to akita store
Apr 26, 2021
f08f610
fix(store): renamed set method to overwrite
Apr 27, 2021
f1d8eb0
fix(store): removed redundant local functions
Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 96 additions & 10 deletions docs/docs/store.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,48 +37,134 @@ When you want to update the store, you can call the `update()` method passing th

```ts title="session.service.ts"
import { SessionStore } from './session.store';

export class SessionService {
constructor(private sessionStore: SessionStore) {}

updateUserName(newName: string) {
this.sessionStore.update({ name: newName });
}
}
}
```

The second `update()` option gives you more control. It receives a `callback` function, which gets the current state, and returns a new **immutable** state, which will be the new value of the store. For example:

```ts title="session.service.ts"
import { SessionStore } from './session.store';

export class SessionService {
constructor(private sessionStore: SessionStore) {}

updateUserName(newName: string) {
this.sessionStore.update(state => ({
name: newName
}));
}
}
}
```

### `set()`

When you don't just want to update the current store state but replace the state completely, you can call the `set()` method passing the new `state`:

```ts title="session.service.ts"
import { SessionStore } from './session.store';
NetanelBasal marked this conversation as resolved.
Show resolved Hide resolved

export class SessionService {
constructor(private sessionStore: SessionStore) {}

updateUserName(newName: string) {
this.sessionStore.set({ name: newName });
}
}
```

As with `update()` the `set()` method also has a second option which gives you more control. It receives a `callback` function, which gets the current state, and returns a new **immutable** state, which will be the new value of the store. For example:

```ts title="session.service.ts"
import { SessionStore } from './session.store';

export class SessionService {
constructor(private sessionStore: SessionStore) {}

updateUserName(newName: string) {
this.sessionStore.set(state => ({
name: newName
}));
}
}
```

You should use `set()` over `update()` when you want to completely replace the current state at the top level.

```ts title="session.store.ts"
type SessionState = SessionStateAuthenticated | SessionStateUnauthenticated;

interface SessionStateAuthenticated {
_tag: 'session-state-authenticated';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using a simple example state and not copy-paste from your application.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a much reduced version of the original, I though that session state was a good simple example and "real world" enough to be useful. My preference when reading documentation has always been simplified "real world" over slightly contrived.

I could simplify the example further to

type SessionState = AuthenticatedState | UnauthenticatedState;

interface AuthenticatedState {
  _tag: 'authenticated';
  uid: string;
}

interface UnauthenticatedState {
  _tag: 'unauthenticated'
}

or even do something like

type SessionState = TokenState | InitialState;

interface TokenState {
  name: string;
  token: string;
}

interface InitialState{
  name: string;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the latter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change to the second option.

user: User;
}

interface SessionStateUnauthenticated {
_tag: 'session-state-unauthenticated'
}

interface User {
uid: string;
displayName: string;
}

```

```json "current state"
{
"_tag": "session-state-authenticated",
"user": {
"uid": "123",
"displayName": "John Doe"
}
}
```

Using the above state model and value of current state, if we call `this.store.update({ _tag: 'session-state-unauthenticated' })` our state will be updated to the following:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need all of this. The method name is straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was concerned that newer users may be unsure when to use each method, but it could be way to verbose.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overwrite name is clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make the change.


```json "using the update() method"
{
"_tag": "session-state-unauthenticated",
"user": {
"uid": "123",
"displayName": "John Doe"
}
}
```

Our value for tag has been updated but the user value from our previous state has stayed, this could lead to unintended side effects so in this case `this.store.set({ _tag: 'session-state-unauthenticated' })` may be a better option. When we use the `set()` method the following is the result:

```json "using the update() method"
{
"_tag": "session-state-unauthenticated",
}
```

We have replaced the previous state completely, leaving behind no unexpected state values.


### `setLoading()`

Set the `loading` state:
```ts title="session.service.ts"
import { SessionStore } from './session.store';

export class SessionService {
constructor(private sessionStore: SessionStore,
constructor(private sessionStore: SessionStore,
private http: HttpClient) {}

async updateUserName(newName: string) {
this.sessionStore.setLoading(true);
await this.http(...).toPromise();
this.sessionStore.update({ name: newName});
this.sessionStore.setLoading(false);
}
}
}
```

Expand All @@ -87,9 +173,9 @@ Set the `error` state:

```ts title="session.service.ts"
import { SessionStore } from './session.store';

export class SessionService {
constructor(private sessionStore: SessionStore,
constructor(private sessionStore: SessionStore,
private http: HttpClient) {}

async updateUserName(newName: string) {
Expand All @@ -98,7 +184,7 @@ export class SessionService {
} catch(error) {
this.sessionStore.setError(error);
}
}
}
}
```

Expand Down
54 changes: 54 additions & 0 deletions libs/akita/src/__tests__/set.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { Store, StoreConfig } from '@datorama/akita';

type ExampleState = ExampleStateA | ExampleStateB;

interface ExampleStateA {
_tag: 'a';
uniqueToA: string;
}

interface ExampleStateB {
_tag: 'b';
uniqueToB: string;
}

const initialState: ExampleState = {
_tag: 'a',
uniqueToA: 'This value is unique to a',
};

@StoreConfig({
name: 'example-store',
resettable: true,
})
class ExampleStore extends Store<ExampleState> {
constructor() {
super(initialState);
}
}

const exampleStore = new ExampleStore();

describe('Store Set', () => {
beforeEach(() => {
exampleStore.reset();
});

it('should set the store value replacing the previous state using a state object', () => {
exampleStore.set({ _tag: 'b', uniqueToB: 'This value is unique to b' });
expect(exampleStore._value()).toBeTruthy();
expect(exampleStore._value()).toEqual({_tag: 'b', uniqueToB: 'This value is unique to b'});
});

it('should set the store value replacing the previous state using a callback function', () => {
exampleStore.set((_) => ({ _tag: 'b', uniqueToB: 'This value is unique to b' }));
expect(exampleStore._value()).toBeTruthy();
expect(exampleStore._value()).toEqual({_tag: 'b', uniqueToB: 'This value is unique to b'});
});

it('should update the store value but only replace specified properties', () => {
exampleStore.update({ _tag: 'b', uniqueToB: 'This value is unique to b' });
expect(exampleStore._value()).toBeTruthy();
expect(exampleStore._value()).toEqual({_tag: 'b', uniqueToB: 'This value is unique to b', uniqueToA: 'This value is unique to a'});
});
});
57 changes: 47 additions & 10 deletions libs/akita/src/lib/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ export class Store<S = any> {

/**
*
* Update the store's value
* Update the store's value, only replacing the specified properties
*
*
* @example
*
Expand All @@ -244,18 +245,49 @@ export class Store<S = any> {
update(state: Partial<S>);
update(stateOrCallback: Partial<S> | UpdateStateCallback<S>) {
isDev() && setAction('Update');
const withHookFn = (curr: S, newS: S) => this.akitaPreUpdate(curr, { ...curr, ...newS } as S);
this._setState(this.prepareNewState(stateOrCallback, this._value(), withHookFn));
}

let newState;
const currentState = this._value();
if (isFunction(stateOrCallback)) {
newState = isFunction(this._producerFn) ? this._producerFn(currentState, stateOrCallback) : stateOrCallback(currentState);
} else {
newState = stateOrCallback;
/**
*
* Set the store's value, replacing the previous value.
*
* @example
*
* this.store.update(state => {
* return {...}
* })
*/
set(stateCallback: UpdateStateCallback<S>);
/**
*
* @example
*
* this.store.update({ token: token })
*/
set(state: S);
set(stateOrCallback: S | UpdateStateCallback<S>): void {
isDev() && setAction('Set');
const withHookFn = (curr: S, newS: S) => this.akitaPreSet(curr, newS as S);
this._setState(this.prepareNewState(stateOrCallback, this._value(), withHookFn));
}

private prepareNewState<S>(stateOrCallback: Partial<S> | UpdateStateCallback<S>, currentState: S, withHookFn: (c: S, n: S) => S): S {
const constructNewState = (x: Partial<S> | UpdateStateCallback<S>, cs: S) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Why do we need a function?
  • Why do we need to create a new function each time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why do we need a function?

I've used a hook function so the prepareNewState method is not bound to a specific class method e.g akitaPreOverwrite or akitaPreUpdate instead a wrapping function of the relevant class method is used in update, overwrite or any future methods. This allows for the same class class overriding of the methods yet still being able to pass them as HOF.
const hookFn = (curr: S, newS: S) => this.akitaPreOverwrite(curr, newS as S)

Using the hook means the ordering of operations can be maintaned.

  1. Why do we need to create a new function each time?

In an effort to make small single purpose functions I like to lexically scope util functions. It does have the possible performance hit but I think the clarity of being able to scroll to the bottom of a method/fn and know what's happening is worth it.

const newState = extractNewState(stateOrCallback, currentState);
const withHook = hookFn(currentState, newState);
return resolveFinalState(currentState, withHook);

This makes it clear I'm extracting the new state from my callback and current state -> apply my hook to the currentState and newState -> finally resolving the final state. I could makes these more descriptive but don't understand what the _producerFn is or why your checking if the state is an object.

You can also pluck out these small functions and raising their scope if you have a need for them elsewhere.

The alternative would be

let newState;
if (isFunction(stateOrCallback)) {
 newState = isFunction(this._producerFn) ? this._producerFn(currentState, stateOrCallback) : stateOrCallback(currentState);
} else {
 newState = stateOrCallback;
};
const withHook = hookFn(currentState, newState);
return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);

While this can be made more clear through comments it could be tricky to understand what the fn/method is doing.

If the nest functions are a concern from performance we could put the functions in private methods, extract them out of the class or of course just remove them and have all the code blocked together.

// Option 1
// Inside the Store class

private extractNewState(x: Partial<S> | UpdateStateCallback<S>, cs: S) {
    if (isFunction(x)) {
      return isFunction(this._producerFn) ? this._producerFn(cs, x) : x(cs);
    } else {
      return x;
    }
  }

  private resolveFinalState(currentState: S, withHook: S): S {
    return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
  }

// Option 2

// Before or after the Store class def

const extractNewState = <S>(x: Partial<S> | UpdateStateCallback<S>, cs: S, _producerFn: (state: any, fn: any) => any) => {
  if (isFunction(x)) {
    return isFunction(_producerFn) ? _producerFn(cs, x) : x(cs);
  } else {
    return x;
  }
};

const resolveFinalState = <S>(currentState: S, withHook: S): S => {
  return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
};

That's my reasoning, happy to do what ever fits best with the project. 👍

Copy link
Collaborator

@NetanelBasal NetanelBasal Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need that constructNewState and resolveFinalState to be local functions?

Why are you not inline them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this:

const extractNewState = (x: Partial<S> | UpdateStateCallback<S>, cs: S) => {
      if (isFunction(x)) {
        return isFunction(this._producerFn) ? this._producerFn(cs, x) : x(cs);
      } else {
        return x;
      }
};

const inCaseClassResolveState= (currentState: S, withHook: S): S => {
   return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
};

const newState = extractNewState(stateOrCallback, currentState);
const withHook = hookFn(currentState, newState);
return inCaseClassResolveState(currentState, withHook);

To be more immediately understandable than:

let newState;
if (isFunction(stateOrCallback)) {
 newState = isFunction(this._producerFn) ? this._producerFn(currentState, stateOrCallback) : stateOrCallback(currentState);
} else {
 newState = stateOrCallback;
};
const withHook = hookFn(currentState, newState);
return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);

If future me or another developer comes along and sees the line of code isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook); it may not make sense without diving in and understanding it. But if the block of code is wrapped in a function called inCaseClassResolveState you get some understanding straight away of what the operation is doing.

With the added benefit of not mutating (with variable reassignment) the newState variable once declared. e.g. using const and not let.

I'm happy to just change the class method to match below if you prefer?

private prepareNewStateAlt<S>(stateOrCallback: Partial<S> | UpdateStateCallback<S>, currentState: S, hookFn: (curr: Readonly<S>, newS: Readonly<S>) => S): S {
    let newState;
    if (isFunction(stateOrCallback)) {
      newState = isFunction(this._producerFn) ? this._producerFn(currentState, stateOrCallback) :    stateOrCallback(currentState);
    } else {
      newState = stateOrCallback;
    }
    const withHook = hookFn(currentState, newState);
    return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comments please, we don't need to create redundant functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes done.

if (isFunction(x)) {
return isFunction(this._producerFn) ? this._producerFn(currentState, x) : x(cs);
} else {
return x;
}
}
const resolveFinalState = (currentState: S, withHook: S): S => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

return isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
}

const withHook = this.akitaPreUpdate(currentState, { ...currentState, ...newState } as S);
const resolved = isPlainObject(currentState) ? withHook : new (currentState as any).constructor(withHook);
this._setState(resolved);
const newState = constructNewState(stateOrCallback, currentState);
const withHook = withHookFn(currentState, newState);
return resolveFinalState(currentState, withHook);
}

updateStoreConfig(newOptions: UpdatableStoreConfigOptions) {
Expand All @@ -267,6 +299,11 @@ export class Store<S = any> {
return nextState;
}

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

ngOnDestroy() {
this.destroy();
}
Expand Down