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

Should state$ already use distinctUntilChanged() ? #497

Closed
jayphelps opened this Issue May 22, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@jayphelps
Member

jayphelps commented May 22, 2018

Right now the new state$ observable doesn't do any kind of checking if the state really did update, so if an action is dispatched but no state updated, state$ will still emit the same state as before. This could be potentially be unexpected and seems like regardless it's undesirable.

Since redux users are supposed to use some sort of immutable data pattern in doing their reducer state updates, we could use distinctUntilChanged() to only notify state$ observers when the state tree has actually changed. The only problem I see is that I have many times seen people not follow 100% immutable patterns, in which case this decision would lead to hard-to-debug stale bugs..but these same bugs would exist in libraries like react-redux which also rely on immutability/shallow comparisons, so I'm inclined to say we should do it.

Thoughts?

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 22, 2018

Cc/ @rgbkrk

@rgbkrk

This comment has been minimized.

Member

rgbkrk commented May 22, 2018

I'm totally cool with building distinctUntilChanged in, especially if that's a similar assumption for react-redux.

@evertbouw

This comment has been minimized.

Member

evertbouw commented May 23, 2018

You always want to do distinctUntilChanged so I agree it's a good idea to build it in. But then users won't be trained to add it everywhere themselves so I think it would be easy to miss elsewhere. Should we ship an operator like select from ngrx? It's basically pipe(map(/* selector */), distinctUntilChanged())

@deanius

This comment has been minimized.

deanius commented May 23, 2018

But suppose you want to provide a zip of {action$, state$}? They no longer match up - got a solution to that?

@evertbouw

This comment has been minimized.

Member

evertbouw commented May 23, 2018

Since currently action$ and state$ emit the same number of items and at the same time, you can use action$.pipe(withLatestFrom(state$)) to achieve the same result.

@jayphelps

This comment has been minimized.

Member

jayphelps commented May 23, 2018

But suppose you want to provide a zip of {action$, state$}? They no longer match up - got a solution to that?

As @evertbouw mentioned withLatestFrom might be what you're looking for, or perhaps combineLatest might be more close to zip except not needing 1:1 emission patterns.

withLatestFrom is going to use the source you apply it to as the signal to continue, e.g. action$.pipe(withLatestFrom(state$)) would use actions as the notifier, emitting an array containing the latest values from both [action, state]; when state$ emits it just updates the internal cache but doesn't use it as a notifier to emit.

combineLatest on the other hand uses any of the provides sources as notifiers, waiting for at least one emission from each but after that no longer synchronized (zip always synchronizes 1:1 emissions from each source)

const somethingEpic = (action$, state$) =>
  combineLatest(action$, state).pipe(
    tap(([action, state) => console.log(action, state)
    ignoreElements()
  );
@deanius

This comment has been minimized.

deanius commented May 23, 2018

Thanks for the idea, and that thoughtful explanation @jayphelps ! Is 'notifier' standard terminology in Rx? Cuz I like it.. (and frankly a lot of the documentation is well, obtuse or too technical)

jayphelps added a commit that referenced this issue May 23, 2018

feat(state$): state$ now only emits subsequent values if the state sh…
…allowly is different (e.g. prevValue !== nextValue). It still emits the current state immediately on subscribe regardless, as it did before, similar to BehaviorSubject. Closes #497

jayphelps added a commit that referenced this issue May 23, 2018

feat(state$): state$ now only emits subsequent values if the state sh…
…allowly is different (e.g. prevValue !== nextValue). It still emits the current state immediately on subscribe regardless, as it did before, similar to BehaviorSubject. Closes #497
@wldcordeiro

This comment has been minimized.

wldcordeiro commented May 24, 2018

This seems like a good optimization. Definitely support putting distinctUntilChanged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment