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.

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

feat(component-store): add config for debounce selectors #2606

Merged
merged 1 commit into from Jul 10, 2020

Conversation

alex-okrushko
Copy link
Member

@alex-okrushko alex-okrushko commented Jul 4, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

By removing debounceSync from a single-value selector, the selector value now becomes available immediately after the updates.

Longer versions:

  • updates/setState were always sync
  • selectors now fall into two buckets:
    • state selectors like selector(state => state.foo) are back to being sync
    • selectors that combine other selectors and/or Observables are still async, scheduled to run into the microtask.

Introduce SelectConfig configuration for selector. Currently it allows to "debounce" selector values until the state is "settled".
By default it will be set to debounce: false, as it makes it more predictable (same behaviour as global store) and allows instant reads from the ComponentStore that would return the value synchronously.

Why this change?

While having both types of selectors async/debounced by default is more performant, it was also:

  • harder to write test for the ComponentStore(s) (on the user side)
  • more confusing for the Developers to understand why the values were not emitted yet,

This last point became more obvious when I saw effects like:

private readonly fooEffect = this.effect<void>(
      obs$ => obs$.pipe(
          withLatestFrom(this.selectedId$),
          switchMap(([, clientId]) => {
            return this.httpApiService.makeACall(clientId);
          }),
          ...

When withLatestFrom is called, the selectedId$ is not ready yet.

The obvious alternative solution would be to provide an imperative way to read the data from the ComponentStore - that solution is also planned and will follow this PR.

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #

What is the new behavior?

Does this PR introduce a breaking change?

[x] Yes
[ ] No

ComponentStore selectors are no longer debounced by default.

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 4, 2020

Preview docs changes for bb376a5 at https://previews.ngrx.io/pr2606-bb376a53/

@alex-okrushko alex-okrushko changed the title feat(component-store): remove debounceSync for single selectors feat(component-store): add config for debounce selectors Jul 8, 2020
@alex-okrushko alex-okrushko force-pushed the cs-sync-selectors branch 3 times, most recently from 249b4fd to 4eb5600 Compare July 8, 2020 20:38
R,
ProjectorFn = (...a: unknown[]) => R
>(...args: O): Observable<R> {
const { observables, projector, config } = processSelectorArgs(args);
Copy link
Member

Choose a reason for hiding this comment

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

I like that the logic of processSelectorArgs is extracted 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Tim 😊

debounceSync(),
map((args: any[]) => projector(...args))
observable$ = combineLatest(observables).pipe(
config.debounce ? debounceSync() : (source$) => source$,
Copy link
Member

Choose a reason for hiding this comment

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

What if we would extract this ternary statement?
Not sure if it's worth it tho, I'm thinking that when this line changes, the line in the if case would also be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I couldn't think of a better way to extract it and make it readable/somewhat easier to understand at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants