component-store: selectors recalculate order and amount cause inconsistencies

[Harpush]

Minimal reproduction of the bug/regression with instructions

https://stackblitz.com/edit/ngrx-seed-xboxzw?file=src/app/store.service.ts
Open console and see the logs

Minimal reproduction of the bug/regression with instructions

Due to combineLatest usage inside component store select each dependent selector emission invokes recalculation. That means even a single update to the reducer can cause multiple selector recalculations with wrong and unexpected data too.
Only using debounce can solve it - but the regular effect problems arise.

The example above creates three selectors:

  public readonly data$ = this.select((state) => {
    console.log('data$', state.data);
    return state.data;
  });

  public readonly hasData$ = this.select(this.data$, (data) => {
    console.log('hasData$', data);
    return data !== undefined;
  });

  public readonly isDataFilled$ = this.select(
    this.hasData$,
    this.data$,
    (hasData, data) => {
      console.log('isDataFilled$', hasData, data);
      // data has `?` for the sake of the example... shouldn't be there
      return hasData ? data?.length > 0 : false;
    }
  );

I would expect on data update that each selector runs once. What actually happens without debounce is isDataFilled$ runs twice for each dependent selector change. Even worse the data will be partial (hasData as true and data as undefined).
The real problem is say with 5 dependent selectors a single update that affects the result of all the 5 selectors will trigger the combined selector 5 times. In regular NGRX store it doesn't work this way but the way I expect.
This kind of bugs are really hard to find and I can't see a good fix for it...

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

NgRx: 13
Angular: 13

Other information

It seems that debounce cannot be avoided... I believe the only way to solve it is debounce always and custom rxjs operator for async select debounced selectors. The only other option is maybe tweaking combineLatest usage... not sure its possible.

I would be willing to submit a PR to fix this issue

• Yes
• No

[markostanimirovic]

Hi @Harpush,

This is the expected behavior of the ComponentStore.select method without debounce. In cases like this, you can add { debounce: true } as the last argument of the select method, so the projector function will be executed when all the state changes within the current microtask occur:

  readonly isDataFilled$ = this.select(
    this.hasData$,
    this.data$,
    (hasData, data) => { /* projector */ },
    { debounce: true }
  );

I will convert this bug report to a discussion because the reported default behavior of the ComponentStore.select method (and combineLatest operator) is expected.

[alex-okrushko]

@markostanimirovic Doesn't that mean that 99% of selector should debounce? Every selector that depends on more than one other selector is at risk here

It doesn't. Rarely all the underlying values change at exact same time. If they do, that means they are relying on the same state/data (and updated at the same time through updater). Like @markostanimirovic said that's exactly the use case for debounce then.

Why is it different from global store?
In a global store a single action can update many reducers at the same time. The selectors in global store are also not returning an Observable directly and require store.select(selector) to do that.
ComponentStore has less scope and less complexity, has a simplified APIs to fit most of the cases to deal with that but it cannot (and should not) replace all of the use cases.

[alex-okrushko]

Btw, the fact that selector in component store works with just Observables and not some kind of other data structure (like MemoisedSelector in global store) allows it to combine data not only from multiple selectors but also from the global store or any other Observable input. I find it pretty powerful, especially for its intended role.

[Harpush]

Up to now it seemed to me most of the global ngrx use cases can be covered by component store.
Anyway due to how select works in component store in those cases I must admit it's tempting to always debounce. This kind of selector must be debounced as you said so a solution for effect usage is required anyway (my other suggestion is irrelevant in this case). I only wonder what's the penalty of doing so - selector wise and effect wise (as it will probably be some forkJoin based solution instead of withLatestFrom)

[alex-okrushko]

For the effect? I provided you a few possible solutions together with stackblitz examples

[Harpush]

Indeed. But looking back I believe your two suggestions might be the best:

1. Add config to effect, e.g. {debounce: true} so that effect won't fire immediately and wait until the next microtask.

2. enrich selector with additional metadata (so it will be Observable & SelectorMetadata return type) AND create a custom operator (e.g. withLatestFromSelector) that would be waiting for the debounced value (based on SelectorMetadata - or not waiting if it's missing, then it's just a withLatestFrom).

This will allow scalability when you need an effect that reads through multiple debounced selectors. This issue made me realize there are selectors that should be debounced always even for effects otherwise it won't be just performance penalty but will actually create bugs.
Can such withLatestFromSelector be achieved with what we currently have (considering I don't need metadata as everything will be debounced)? Or it anyway requires NGRX additions? And are there performance or other issues with debouncing everything and using this approach in effects?

[Harpush]

@alex-okrushko I ended up doing this:

1. Debounce everything for consistency, performance and this bug solving
2. Each effect that uses selectors from store add observeOn(asapScheduler) and wrap it with forkJoin as concatLatestFromSelector to allow effects to use debounced selectors as expected
3. In component I am using the vm$ approach aggregating with combineLatest all the debounced selectors and using ngIf around the whole template. This solves an issue as async pipe first emission is null in case of debounce so currently the UI waits for it

My question is: Is that an applicable solution? Are there any pitfalls I can't see right now? I will gladly hear your opinion.
For reference this is concatLatestFromSelector implementation:

observeOn(asapSceduler),
concatMap(value => forkJoin([of(value), this.someSelector$.pipe(first())]))