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(effects): concatLatestFrom operator #2760

Merged
merged 1 commit into from
Feb 2, 2021

Conversation

david-shortman
Copy link
Contributor

@david-shortman david-shortman commented Oct 25, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] 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?

The NgRx Effects docs tell us:

Note: For performance reasons, use a flattening operator in combination with withLatestFrom to prevent the selector from firing until the correct action is dispatched.

An operator which does this for consumers of NgRx is nice to have, but does not currently exist.

Closes #2222

What is the new behavior?

  • Calls to store.select (or any arbitrary function that produces an observable) are delayed until the function provided to concatLatestFrom is called
  • The lazy function can pass the current value in such that you can select using a selector factory like so:
concatLatestFrom((action) =>
        [this.store.select(getInfo(action.infoId)),
        this.store.select(selectOtherData)],
      ),

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@ngrxbot
Copy link
Collaborator

ngrxbot commented Oct 25, 2020

Preview docs changes for 6e6e146 at https://previews.ngrx.io/pr2760-6e6e1463/

@david-shortman david-shortman changed the title RFC: Introduce lazyWithLatestFrom operator (name pending) RFC: feat(effects) introduce lazyWithLatestFrom operator (name pending) Oct 25, 2020
Copy link
Member

@alex-okrushko alex-okrushko left a comment

Choose a reason for hiding this comment

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

@david-shortman
Thanks for the PR. 👍

  • Could you also provide the support for a single select without wrapping it into tuple?
  • as for the naming (typically the hardest part 🙂), what do you think about withLazyLatestFrom?

@david-shortman david-shortman changed the title RFC: feat(effects) introduce lazyWithLatestFrom operator (name pending) RFC: feat(effects) introduce concatLatestFrom operator (name pending) Oct 27, 2020
@david-shortman
Copy link
Contributor Author

@alex-okrushko Have achieved allowing single observable or array of observables to be returned by factory function.

I kept mulling the name over in my head, and I wonder if concatLatestFrom accurately represents what the operator does?

@david-shortman david-shortman force-pushed the lazy-with-latest-from branch 3 times, most recently from b6d1589 to 2d85933 Compare October 27, 2020 14:34
@david-shortman
Copy link
Contributor Author

david-shortman commented Nov 5, 2020

@alex-okrushko benlesh over at RxJs gave a pretty good suggestion to use defer instead within withLatestFrom: ReactiveX/rxjs#5845 (comment)

Wondering if the better direction would be updating the docs to use this simpler way to delay running store.select?

Of course, withLatestFrom + defer does not handle flattening an array of observables like the proposed concatLatestFrom does. This PR could also use defer under-the-hood instead

@srnec
Copy link

srnec commented Nov 6, 2020

What about creating just a wrapper function?

export function withLazyLatestFrom(...observables: Observable): (source: Observable) => Observable {
    return (source: Observable) => {
        return source.pipe(concatMap((value) => of(value).pipe(withLatestFrom(...observables))));
    };
}

Typedefs omitted because I am not able to type it properly in order to infer all the types properly.

@david-shortman
Copy link
Contributor Author

@srnec your solution does not delay the evaluation of the input observable. The argument to the operator will be immediately evaluated unless you use a lambda and evaluate the lambda later.

@alex-okrushko
Copy link
Member

@david-shortman
I don't think defer helps here.
It works same as without it. https://stackblitz.com/edit/angular-mpsnhs?file=src%2Fapp%2Feffects.ts

defer delays the evaluation until subscription, not until the value is pushed through the pipe - at least that's my understanding.

@david-shortman
Copy link
Contributor Author

@alex-okrushko ah, ok. I'll see what the RxJs folks think.

Side-note, what would be the best path forward for this PR be TS version-wise? Do I need to target it to pre-V4, or just let this hang around while?

@alex-okrushko
Copy link
Member

@alex-okrushko ah, ok. I'll see what the RxJs folks think.

Side-note, what would be the best path forward for this PR be TS version-wise? Do I need to target it to pre-V4, or just let this hang around while?

TS 3.9, if possible.
TS 4.0 is fine. Angular will have 4.0.x for v11, and we can release this operator with v11 as well.

@david-shortman david-shortman changed the title RFC: feat(effects) introduce concatLatestFrom operator (name pending) RFC: feat(effects) introduce concatLatestFrom operator Nov 25, 2020
@david-shortman david-shortman changed the title RFC: feat(effects) introduce concatLatestFrom operator RFC: feat(effects) concatLatestFrom operator TS 4.X Nov 25, 2020
@david-shortman david-shortman marked this pull request as ready for review December 11, 2020 23:17
@david-shortman david-shortman changed the title RFC: feat(effects) concatLatestFrom operator TS 4.X RFC: feat(effects) concatLatestFrom operator Dec 11, 2020
@david-shortman david-shortman force-pushed the lazy-with-latest-from branch 3 times, most recently from 47bb85c to 9136797 Compare December 12, 2020 01:13
@david-shortman david-shortman changed the title RFC: feat(effects) concatLatestFrom operator feat(effects) concatLatestFrom operator Dec 15, 2020
@david-shortman david-shortman force-pushed the lazy-with-latest-from branch 3 times, most recently from c65cda0 to 0f2b0e1 Compare December 16, 2020 01:11
@brandonroberts
Copy link
Member

@david-shortman the API docs aren't showing in the preview. Will you take a look?

@david-shortman david-shortman force-pushed the lazy-with-latest-from branch 3 times, most recently from 9aa79f2 to da63bcd Compare December 30, 2020 00:58
@brandonroberts
Copy link
Member

@david-shortman I'll take a look this week

@brandonroberts
Copy link
Member

I'm not sure why this isn't showing in the PR preview for the API docs, but the changes LGTM and I think its ok to move forward with. I'll wait on @alex-okrushko or @timdeschryver to provide another review/approval before merging.

@alex-okrushko alex-okrushko changed the title feat(effects) concatLatestFrom operator feat(effects): concatLatestFrom operator Jan 31, 2021
Copy link
Member

@alex-okrushko alex-okrushko left a comment

Choose a reason for hiding this comment

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

@david-shortman

Fantastic work, David! Thank you so much!

Sorry that it took a bit longer than expected.
Let's address these comments and we'd be able to get it into v11, which hopefully can be release within next week or two.

@brandonroberts 😉

modules/effects/src/concat_latest_from.ts Outdated Show resolved Hide resolved
modules/effects/src/concat_latest_from.ts Outdated Show resolved Hide resolved
modules/effects/src/concat_latest_from.ts Outdated Show resolved Hide resolved
modules/effects/src/concat_latest_from.ts Outdated Show resolved Hide resolved
modules/effects/spec/concat_latest_from.spec.ts Outdated Show resolved Hide resolved
>(
observablesFactory: (value: V) => T extends Observable<unknown>[] ? [...T] : T
) {
return concatMap((value) => {
Copy link
Member

Choose a reason for hiding this comment

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

Btw, when I write custom operators, I typically do

function myOperator(foo: string): (source: Observable<A>) => Observable<b> {
   return source.pipe(
   .... // here goes the implementation
   )
}

I don't quite remember why that's the way to do it. E.g. @cartant also uses it in his article: https://ncjamieson.com/managing-operator-state/

Might be a good idea to do it this way anyway. (in the meantime, I'll try to find out why it's done so).

e.g. here's what I also arrived at for this operator: https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbzgeQEYGcCmUBuBDVAG0wBoUNt8jMBJAOzAFcYy0tdMATANT0MczIAZmQhC4AXzhCoEEHADkUAB4ArdAoDcAKFCRYiOAGMIdI3hgBZPGDIB3YDAAWAGQuZ0MAGKz5UmXKKKuoA9BBg2BbQGjraISFwAIJQUHgAnnAQONiEEHiccMDocHSYXFzSwFCeAHTamMr68EKMZjDApsam5jBuMB7evgA8ACpwDf10nMVslATEQ60A1nQQdnQAfADaALpk3BsAFNpwmRS48x5eeEYw0GkAXHCH+PyYT9wAlHAAvBtwWxqQJGOzg8Tg6DAUEw+WAdAA5hCIHA7JhjHg6HB4Zh4M40TBGGBiJkoJxsJlxJhiCBMHQYOhtJ8nsdTugIIwoEZ3uR2FQFgdGb9-rMLtQhltuGQgTUkFsANKFTFLTBpMRwEFPEXZHh8ATCUbynb-CRGnQNJrSVq3DqYkxmCx9AY+OSjcbKSbTHlzMXLVbrDb7I4nM68y7oa63e7M14CD7fP7qxnMtkcrma858zBDA7x4UZy7iyVejg6t76kYbU1xABU1ZO1cUdp6js8zpACi6IFQcI8cDxSNTaJjmHrcAxBX7hDwnjH+GAU+ocGH0l8Y7gU4AXvOMpgY+4ClrLqO4WvSnYxyl0vWQvVGtBmlb2p0mw73K3hsGxhNaZ7Dz66CsaybHAAA+xaZosAF+psuwkMGkrBgASr8ALBqcCGnKc0qHF+7o-sUySpGkkGAf6aGYQA-IY8qKnAyqquIGrgVwvBlkIBpykakjkacTxbCCnzBjs2hBqcED5tQ4Y3HcUCPM8w5xkKiZMs8Kactyf78hsubgQWiH-AgwbQgSUCYocalprpYo5kK5EWZgNRgMAEQsphpwvlYNiHC8uqYDphluW5dozuJoaSShoXesQUmRrJPlvJ8OiBZhwXwJForRYk6CEekKE5WkNRFPlhzpZm6CCclblUSVEnRWOMy1bQDDMCR0HbDsFWVbxAKlWGOxJZVxkcpiYjxQInyOc5mCuV1DjOC2gxyIc0q9ZJWX5Z1yXfNOVkLPpA1uRIm1wIl2gSEAA

Copy link
Contributor Author

@david-shortman david-shortman Jan 31, 2021

Choose a reason for hiding this comment

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

Since writing this PR, I have spent more time on the RxJs docs site and found that they give the same another different recommendation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revising my statement- RxJs now provides the pipe function to independently and more quickly create custom operators. No need to spend the extra characters referring to a source and writing details that expose a deeper understanding than necessary of the nature of observables.

@alex-okrushko alex-okrushko merged commit 55f0f7a into ngrx:master Feb 2, 2021
@alex-okrushko
Copy link
Member

Awesome work, @david-shortman 👍

@david-shortman david-shortman deleted the lazy-with-latest-from branch February 3, 2021 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Change behavior of ofType in Effects to be more consistent when combining observables
5 participants