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): add lazy concatLatestFrom operator for TS 3.9 #2799

Closed

Conversation

david-shortman
Copy link
Contributor

@david-shortman david-shortman commented Nov 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 would be 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

Other information

  • I have opened a separate PR with much simpler types for TS 4.X

@david-shortman david-shortman changed the title Concat latest from 3.9 feat(effects): add lazy concatLatestFrom operator for TS 3.9 Nov 25, 2020
@ngrxbot
Copy link
Collaborator

ngrxbot commented Nov 25, 2020

Preview docs changes for 15390a3 at https://previews.ngrx.io/pr2799-15390a30/

@david-shortman
Copy link
Contributor Author

Fun(?) note:

In the series of function overload declarations, the declaration that comes last MUST come last.

I was losing my sanity trying to figure out why the types weren't working when it was put at the top. Apparently, when TS attempts to check if a function call matches a signature, it would refuse to believe an array could be narrowed to any of the tuples in the signatures.
Moving the function signature that uses no array for the observable factory to the bottom fixed this but I don't know why.

@jeetparikh
Copy link

hey @david-shortman great to see this implementation :) I had this question around using withLatestFrom inside of effects to get state. Is this withLatestFrom always listening when the state slice it is referring to updates?
I have tried using a .tap operator on it and I see it always logs when the state slice updates even if that particular effect is not running. So it is always listening? or I am misunderstanding something and relates to concept of hot/cold observables where it's logging cos I am using the tap operator on it. Thanks :)

@david-shortman
Copy link
Contributor Author

david-shortman commented Dec 3, 2020

hey @david-shortman great to see this implementation :) I had this question around using withLatestFrom inside of effects to get state. Is this withLatestFrom always listening when the state slice it is referring to updates?
I have tried using a .tap operator on it and I see it always logs when the state slice updates even if that particular effect is not running. So it is always listening? or I am misunderstanding something and relates to concept of hot/cold observables where it's logging cos I am using the tap operator on it. Thanks :)

withLatestFrom is an RxJs operator which concats the latest value of the provided observables onto a source observable. It has no relationship to the NgRx Store or Effects library.

In the following image, the source observable is the first stream. The second stream is the input observable which we want the latest value from. The bottom stream is the output, which emits a value every time the source stream emits a value, and attaches (concats) the latest value from the second stream to it.

I am not sure in what way you are using tap. Let's say you have an effect like

someEffect = createEffect(() =>
  this.actions.pipe(
    ofType(SomeAction),
    withLatestFrom(this.store.select(selectSomething)),
    tap(() => console.log('effect triggered'))
);

then 'effect triggered' would only print when SomeAction was dispatched. withLatestFrom has no effect on when the function provided to tap is called in this instance.

concatLatestFrom has the same marble diagram as seen above.

The reason for creating concatLatestFrom instead of withLatestFrom is that the selector code in selectSomething's projector will begin being re-evaluated the moment the effect is created. Using concatLatestFrom delays the selector execution until you actually reach the point at which it should be triggered (in the above example, until an action of SomeAction type is received).

@jeetparikh
Copy link

hey @david-shortman thank you for the great explanation.
My bad I should have posted a snipped. Here it is

@Effect()
    fetchSomething$ = this.actions$
      .pipe(
        ofType<FetchSomethingAction>(FETCH_SOMETHING),
        withLatestFrom(
          this.store$.select(getSomething)
            .pipe(
              tap((_) => console.log('this always executes even if effect is not triggered')) // <--- THIS TAP HERE
            )
        ),

So in this example above, the log inside tap happens everytime the state slice referred by 'getSomething' updates even if the 'FetchSomethingAction' is not dispatched. Even if I use a flattening operator like 'concatMap' it does the same thing... Am I missing something? Thanks :)

@david-shortman
Copy link
Contributor Author

david-shortman commented Dec 3, 2020

So in this example above, the log inside tap happens everytime the state slice referred by 'getSomething' updates even if the 'FetchSomethingAction' is not dispatched. Even if I use a flattening operator like 'concatMap' it does the same thing... Am I missing something? Thanks :)

So when you use withLatestFrom, it would be expected that the console.log would occur immediately upon the declaration of the effect, as you observed.

Using a flattening operator (or concatLatestFrom), the tap should not be executed because the store.select will also be prevented from executing until a value from the source observable triggers evaluation of the lambda in the flattening operator.

My hypothesis for why you're seeing a tap is that, once you've set up a pipe from the observable produced by store.select, the selector's value continues to change in your application, and that tap continues to be executed for each change because it was part of an observable that was subscribed to. This would only occur after the effect has run once, however.

That hypothesis sounds kinda wrong tho. To continue talking about this stuff since it's not entirely relevant to the PR, if you want you can open a StackOverflow question and we can converse about it there.

@jeetparikh
Copy link

hey @david-shortman thank you for getting back. The reason I asked this question here is if that was something it could be addressed along with the PR.

My hypothesis for why you're seeing a tap is that, once you've set up a pipe from the observable produced by store.select, the selector's value continues to change in your application, and that tap continues to be executed for each change because it was part of an observable that was subscribed to.

Does this mean the log happens only because there is a subscription? or the withLatestFrom is triggering/listening irrespective? My concern around this is we use quite of withLatestFrom inside effects and with some effects in a large application is this is a performance concern?

Again, raising this here along with the PR is because the PR is related to improving how some behaviour around this is currently handled in NgRx.

But if you think this isn't the right place, I can create a question on StackOverflow.

Thanks

@david-shortman
Copy link
Contributor Author

david-shortman commented Dec 4, 2020

Does this mean the log happens only because there is a subscription? or the withLatestFrom is triggering/listening irrespective? My concern around this is we use quite of withLatestFrom inside effects and with some effects in a large application is this is a performance concern?

There's just a lot of nuance that has to be described to give a complete picture. And describing that nuance is what is irrelevant to the PR 😅.

Also, withLatestFrom is owned by RxJs, not NgRx, so I can't do anything to fix that.

Here's a nuanced explanation:

Concept 1- How a Javascript engine will evaluate the declaration of an Observable with a pipe

Say you have an observable with a pipe like this

someObservable.pipe(
  filter(value => value !== null)
  map(value => value * 2)
  tap(value => console.log(value))
  withLatestFrom(this.store.select(selectSomething))
  concatLatestFrom(() => {
    console.log('now I will select using the selector');
    return this.store.select(selectSomething);
    })
  );

The expression will be evaluated like so-

  • someObservable will be looked up in the current context
  • The property pipe will be retrieved from someObservable
  • Since pipe is a function call, each of its parameters will be evaluated first before executing it
    • filter is a function call, so each of its parameters will be evaluated
      • The first and only parameter to filter is a lambda expression. The lambda will not be executed yet- on its own, the lambda expression evaluates to a value (a Function which has parameters and a code block)
      • filter is executed with its parameter. This merely returns an OperatorFunction (which is later used by the internals of RxJs). The input lambda is not executed within the internals of filter.
    • Then similarly map and tap will have their arguments evaluated and return OperatorFunctions. They are evaluated like filter
      • Note this means that the console.log in the tap will not yet be executed because the lambda itself is not yet executed
    • withLatestFrom's parameters are evaluated
      • this.store.select(selectSomething) is executed. Now NgRx will start tracking changes to the store and re-evaluating selectSomething's projector each time the arguments to it change.
      • withLatestFrom is executed using its parameters (again just returns an OperatorFunction which doesn't "do" anything with its parameter yet)
    • concatLatestFrom's parameters are evaluated
      • The parameter is just a lambda, so again the statements inside the lambda are not evaluated
      • concatLatestFrom then returns an OperatorFunction
  • Finally, pipe is executed with its parameters. It returns an Observable (but doesn't do anything with the operators yet).

Okay, so that was a long list, and some important distinctions were made for what gets executed.

Notice that so far, we've only declared an Observable. We haven't subscribed to it yet.

That means, on its own, if nothing subscribes to it, then the internals of RxJs will never use the OperatorFunctions that were produced, and the lambdas that were provided to the operator definitions will also never be called.

Concept 2- Effects are just Observables

In Angular, you initialize an Effects class by providing it to something like

MyAppModule {
  // ... other stuff
 imports: [
   EffectsModule.forRoot(MyEffects)
 ]
 // ...other stuff
}

This tells NgRx to subscribe to every public Observable declared on the class MyEffects

The createEffect function (or the @Effect annotation in older versions of NgRx) adds some other config data to the the Observable (like whether NgRx should dispatch the values output by the Observable).

Your "effects" that you write in Effect classes are just Observables, there's nothing else special about them. That means that the definition you write for the Observable is merely that, a definition.

NgRx subscribes to them, which then means as the source Observable (usually this.actions) emits values, that stream is transformed into some output.

Those Observables create side effects in 2 ways:

  • The output of the Observable can be dispatched (unless you provide the config { dispatch: false } to createEffect
  • Code that executes in operators like tap can create side effects in the application or on some external service (like making an HTTP call)

Concept 3- Effects are evaluated just like described by Concept 1

When your effect class is initialized, the declaration of each Observable is evaluated. This results in the arguments to operator creator functions being evaluated. So for instance, any usages of withLatestFrom will result in this.store.select immediately being called, and NgRx will start evaluating the selector projector functions.

The effects will then be subsequently "triggered" because they are subscribed to by NgRx.

Conclusion

With these principles in mind, hopefully it is now understandable why withLatestFrom can have a performance impact. The argument to it is not a lambda, and therefore is immediately evaluated when the Effects class is initialized. Then, if your selector (or series of composed selectors) are expensive to evaluate, they are evaluated unnecessarily.

As to using tap on the Observable produced by this.store.select itself... I leave it up to the reader to combine the principles above to come up to a hypothesis as to when it would be executed.

@jeetparikh
Copy link

Thank @david-shortman for the explanation. This is very detailed 👍 I can see why it is a nuance.
Highly appreciate you taking time for such a detailed explanation :) thanks

@david-shortman
Copy link
Contributor Author

@alex-okrushko I didn't know if I needed to do anything else to mark this PR as ready for review... so I'm just throwing out that this PR is ready for review 😅

@markostanimirovic
Copy link
Member

@david-shortman
The version of TypeScript on master branch is now 4 🙂

@david-shortman
Copy link
Contributor Author

Oh yeah look at that. I'll close this and move my documentation updates to #2760

@david-shortman david-shortman deleted the concat-latest-from-3.9 branch December 12, 2020 02:49
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.

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