ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
7.96k stars 1.95k forks source link

component-store: Make selectors debounce better and easier to use #3507

Open Harpush opened 1 year ago

Harpush commented 1 year ago

Information

Currently debounce can be used by passing to select { debounce: true }. That means a selector must be either debounced or not. This creates a few problems:

  1. Any debounced selector cannot be used easily in effects
  2. Any selector based on debounced selector is actually kind of debounced too - but not always how you would expect. That makes even one debounced selector a dangerous thing.

After working with component store for some time I realized what I was missing is having the ability to both debounce and not debounce the same selector. Any selector used in a component should be debounced for performance while selectors used in effects shouldn't. That creates a contradiction with the current implementation as one selector can be used in a component as well as in an effect.

The proposed solution is allowing to debounce every selector while also allow immediate access to any selector. I came up with a solution that wraps component store and I believe it's a good addition. The idea is returning an object from select containing two properties debouned$ and immediate$ allowing the user to decide which strategy to use solving the above problems.

The implementation is pretty straightforward: First selector:

{
  immediate$: this.select(projectionFn),
  debounced$: this.select(projectionFn, { debounce: true }),
};

Selectors based on other selectors:

{
  immediate$: this.select(
    ...selectors.map((selector) => selector.immediate$),
    projectionFn
  ),
  debounced$: this.select(
    ...selectors.map((selector) => selector.debounced$),
    projectionFn,
    { debounce: true }
  ),
};

Considering we can memoize projectionFn once for both selectors returned this solution has many benefits:

  1. No need anymore to think if debounce can be passed and what effect will it have on other selectors and effects.
  2. Debounce performance boost to any selector used by a component
  3. Easy effects access to immediate store value
  4. Due to memoization, projectionFn will always evaluate once although there are two selectors.
  5. Due to select internal shareReplay using refCount if you don't use any of the selectors in the object there will be no ghost subscriptions.

The only thing lost here is the ability to pass any observable to select. This can be solved anyway. One option is having this functionality as a new method called for example createSelector and retaining the select functionality. This will also make this change non breaking.

Describe any alternatives/workarounds you're currently using

  1. Carefully use debounce as it is now - from my experience it can lead to a lot of weird results especially when combining debounced and non debounced selectors.
  2. Don't use debounce at all and pay the performance penalty (and actually the memoization penalty when using async pipe inside ngIf due to refCount).
  3. Wrap component store by yourself with this functionality

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

Harpush commented 1 year ago

In my opinion, the selector should provide the ability to read a value from store, but it shouldn't control when the value gets emitted. Instead, it's better for the consumer of the selector to control when it listens. You can always create a reusable RxJS pipe if multiple consumers need to do this the same way like:

const userPreferences$ = pipe(
   select(fromUserSelectors.selectUserPreferences),
   debounce(() => interval(250))
)

In effects, if you need the latest value from a selector, you can use the concatLatestFrom in the effect pipeline.

updateUserTheme$ = createEffect(() => {
  return this.actions$.pipe(
     ofType(fromUserPreferencesActions.updateUserTheme),
     concatLatestFrom(() => this.store.select(fromUserSelectors.selectUserPreferences)),
     concatMap(([action, userPreferences]) => {
        // some logic
     })
  )
})

If you need to, you can also use the custom rxjs pipe here, too, but it's generally a bad idea, especially with chatty state since it can cause back pressure in the effects pipeline.

Your state store should always be the source of truth. If you routinely find yourself trying to control when a new value is emitted from a selector, it's probably a good idea to reevaluate when you're actually updating state. Make sure you're not updating state prematurely or unnecessarily and causing selectors to emit bad values.

I provide Angular consulting to teams on the side and they usually contact me because they have a problem they don't know how to resolve. Trying to control when selectors emit the value they want to listen to is one of the most common reasons I am contacted. I've always solved that problem for them by shifting their focus from "when should I listen to the emitted value from the selector" to "when should state be updated, causing the selector to emit". This shift in focus results in their state store always being the source of truth

Your examples are about the global store and this issue is about component store. In the global store the debounce flag does not exist as you can listen to the same action in multiple places thus forming a single update and a single emission (it has its own SRP problems though...). In ComponentStore you don't have this privilege as two updates to two different ComponentStores can't be batched. That's why ngrx introduced the debounce flag. This issue is about making the debounce flag easier to use and less error prone.

KylerJohnsonDev commented 1 year ago

@Harpush Oh, right. My mistake. I removed my comment so that it doesn't distract from any discussion about this issue. I'm sorry for the trouble.