Platform: ComponentStore: Support passing "Partial<T> | Observable<Partial<T>>" to patchState

[jaufgang]

One of the things I love about ComponentStore is that Updaters can accept a value or observable.

Another one of the things I love about component store is the patchState function, which accepts Partial<T> , so I can tersely update values in the store without explicitly creating an updater function

What would be even more awesome, is if patchState could also accept Observable<Partial<T>> similar to how the updater function does, so I could write this:

interface FooComponentState {
  foo: Foo;
  bar: Bar;
}
export class FooComponent {
  @Input() set bar(bar: Bar) {
    this.store.patchState({ bar });
  }

  constructor(
    public fooService: FooService,
    public store: ComponentStore<FooComponentState>,
  ) {
    this.store.setState({ foo: undefined, bar: undefined });
    this.store.patchState(fooService.foo$.pipe(map(foo => ({ foo })))); //😀
  }
}

Describe any alternatives/workarounds you're currently using

interface FooComponentState {
  foo: Foo;
  bar: Bar;
}
export class FooComponent {
  @Input() set bar(bar: Bar) {
    this.store.patchState({ bar });
  }

  constructor(
    public fooService: FooService,
    public store: ComponentStore<FooComponentState>,
  ) {
    this.store.setState({ foo: undefined, bar: undefined });
    this.store.updater<Foo>((state, foo) => ({
      ...state,
      foo,
    }))(fooService.foo$); //😢
  }
}

Other information:

If accepted, I would be willing to submit a PR for this feature

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[markostanimirovic]

I think it's late to add this in v11, but looks good to me. If the team agrees, I'll create PR for this feature in the future.

[alex-okrushko]

So here are some thoughts:
I am still unsure whether adding updater(...)(Observable) was a good "philosophical" decision. e.g. that introduces asynchronous behavior into something that should be sync. E.g. historically in NgRx all the async things were delegated to the effects while reducers (and updaters are like individual on-cases of reducers) were supposed to be sync.

If we want to keep updater(...)(Observable) option then we could add patchState(Observable) as well... Furthermore, we could add setState(Observable) as well - but that doesn't make any sense to me.
Alternatively, we should remove updater(...)(Observable) option.

(definitely late for v11)

Thoughts?

[markostanimirovic]

If we want to keep updater(...)(Observable) option then we could add patchState(Observable) as well...

👍

Alternatively, we should remove updater(...)(Observable) option.

Yes, it would be reasonable decision, because removing updater(...)(Observable) will prevent copying values from another source of observable data to the component store's state, which is bad practice 🤔

[jaufgang]

I don't understand why copying values from another source of observable data to the component store's state should be considered bad practice and feel that ComponentStore would be diminished by removing the ability to pass observables to updaters.

Since I discovered ComponentStore, I've been developing a pattern for for using ComponentStore as a cornerstone to building components that are fully reactive, and documenting the pattern in a style guide. This guide recommends using the ComponentStore as part of the component pattern to store _all independent state properties_ of every _smart_ component in a project. (The Service extending ComponentStore pattern is recommended for sharing state between related components that work together to implement a feature, and both patterns are used together side-by-side)

The state properties for a component that get injected into it's store can come from various sources, including user interactions through event handlers, inputs using setters and patchState() (as shown above) and injected services (also as shown above), and that could include other shared stores.

The key concept here is that each component's store always holds _all_ of the independent state properties of interest to itself. Derived state properties are generated using selectors that _only select from the store_ and use projector functions to compute derived values, and then a viewModel observable vm$ is composed by combining store.state$ with all the derived state selectors using combineLatest().

So in the example I used above, copying values from another source of observable data (from an injected service) into the component's store using patchState is effectively declaring that observable data to be part of the state of this component. The same fooService might expose many additional properties or functions that may or may not be observable but are not of interest to the current component, so they get ignored by not getting patched into the component's state and are not used or referenced by any other part of the component.

In practice I've found this pattern to be very elegant and functional, providing complete clarity into a component's workings. A new developer reading this code can see exactly what is relevant to the component, and where it comes from. It makes it easy to reason about the component's behaviour and inspect its current independent or derived state at any time (eg by subscribing to state$ or vm$ and logging to the console, or using <pre>{{store.state$|async|json}}</pre> during development.)

I really don't think that should be considered bad practice.

[jaufgang]

@alex-okrushko, to respond to your "philosophical" question more directly, I'd simply say that despite some parallels, ComponentStore is fundamentally a different type of beast than NgRx Store. NgRx Store implements a publish-subscribe pattern that is not present in ComponentStore. The absence of an action stream in ComponentStore removes some constraints on its architecture, and opens possibilities for more flexibility. I just think of ComponentStore as a super-convenient tool for managing component state reactively in a centralized and consistent manner. That convenience is facilitated by functions for writing and reading data to and from the store (setState/patchState/updaters/selectors) and the more streamlined these functions are, the more convenient they are to use.

[markostanimirovic]

Let me explain what I meant when I said 'bad practice'.

So, in the example that you provided:

interface FooComponentState {
  foo: Foo;
  bar: Bar;
}

export class FooComponent {
  @Input() set bar(bar: Bar) {
    this.store.patchState({ bar });
  }

  constructor(
    private fooService: FooService,
    private store: ComponentStore<FooComponentState>,
  ) {
    this.store.setState({ foo: undefined, bar: undefined });
    this.store.updater<Foo>((state, foo) => ({
      ...state,
      foo,
    }))(fooService.foo$); //😢
  }
}

There are two stateful services. First is store and second is fooService. Every time when fooService.foo$ emits a value, foo slice from store will be updated. So here, you have the same state slice in two sources.

What could be a problem here?

Well, foo slice as a part of store's state may cause potential bugs, because it could be changed like any other slice from another updater. However, expected behavior is that it should be always the same as fooService.foo$.

So, in my opinion, better way is to simply combine external observable with component store's state by using select method and expose derived observable as a view model to the component's template:

interface FooComponentState {
  bar: Bar;
}

export class FooComponent {
  @Input() set bar(bar: Bar) {
    this.store.patchState({ bar });
  }

  readonly foo$ = this.fooService.foo$;
  readonly bar$ = this.store.select(state => state.bar);

  readonly vm$ = this.store.select(
    foo$,
    bar$,
    (foo, bar) => ({ foo, bar }),
  );

  constructor(
    private fooService: FooService,
    private store: ComponentStore<FooComponentState>,
  ) {
    this.store.setState({ bar: 'baz' });
  }
}

With this approach, template can use vm$ to get all the data that it needs and you don't have to worry about the foo slice being accidentally changed.

[alex-okrushko]

So you are actually both right.

There are two patterns here, but one is thing is true for both of them: the Service with reactive property (FooService in this case) is the source of truth of the "shared/persisted state".

Pattern 1

we use that value to derive data, then it will look like this:

interface FooComponentState {
  bar?: Bar;
}

class FooComponentStore extends ComponentStore<FooComponentState> {
  constructor(
    private readonly fooService: FooService,
  ) {
  super({});
  }

  readonly bar$ = this.store.select(state => state.bar);

  readonly vm$ = this.store.select(
    fooService.foo$, // 👈  provides data for the new derived state
    bar$,
    (foo, bar) => ({ foo, bar }),
  );
}

export class FooComponent {
  @Input() set bar(bar: Bar) {
    this.componentStore.patchState({ bar });
  }

  readonly vm$ = this.componentStore.vm$;

  constructor(private readonly componentStore: FooComponentStore) {}
}

Notice here the foo never becomes as part of the ComponentStore's state.

Pattern 2

The Service's data (foo in this case) feed the ComponentStore, but it becomes local state until some action is taken to persist it to the shared one.

For example, think of some Form data that is fed from the "shared / persisted state" and can be manipulated by the user in the form, but is not persisted/pushed upstream until user clicks "Save" button.

interface FooComponentState {
  bar?: Bar;
  foo?: Foo;
}

class FooComponentStore extends ComponentStore<FooComponentState> {
  constructor(
    private readonly fooService: FooService,
  ) {
  super({});
  fooUpdater(this.fooService.foo$) // 👈  if data changes from a persisted source it always updates the state
  }

  readonly fooUpdater = this.updater((state, foo) => ({...state, foo}));
  readonly bar$ = this.store.select(state => state.bar);

  readonly vm$ = this.store.select(    
    ({foo, bar}) => ({ foo, bar }), // 👈  both foo and bar are from state (this is basically this.state$)
  );

  readonly persistData = this.effect<void>(trigger$ => {
    return trigger$.pipe(
      concatMap(() => {
         // this actually works 👇  this.get() is part of the API 🙂
         const localFooValue = this.get().foo; 
         return this.fooService.updateFoo(localFooValue); // 👈  persist the value upstream.
      }),
    )
  });
}

export class FooComponent {
  @Input() set bar(bar: Bar) {
    this.componentStore.patchState({ bar });
  }

  readonly vm$ = this.componentStore.vm$;

  iUpdateLocalFoo(newFoo: Foo) {
    this.componentStore.fooUpdater(newFoo); // 👈  something can update LOCAL state foo
  }

  iSaveFoo() {
    this.componentStore.persistData(); // 👈  updates foo upstream. No longer just local state, and it's ready to be persisted.
  }

  constructor(private readonly componentStore: FooComponentStore) {}
}

In this case the patchState(Observable) can become useful.

I just worry that it will be misused in cases where pattern 1 should be used, where it should not be part of the state.