kosich / rxjs-autorun

Re-evaluate an expression whenever Observable in it emits
MIT License
33 stars 2 forks source link

Why the distinctUntilChanged? #21

Closed Jopie64 closed 3 years ago

Jopie64 commented 3 years ago

In commit 613224cab28ba6c0a9a5781eec9e003065e1874f, two distinctUntilLatest operators were added... I was wondering why that is..? I have some use cases where it doesn't have any effect because it is used on reference or array types. And it doesn't perform a deep compare by default. Also I could imagine some use cases where you don't want it to block non-changes. It could be a signal to update something regardless of whether it changed or not. Also, a user can add it him/herself, even with deep compare, when needed...

Would it be a problem to remove it? Or is there a good reason to leave it there?

kosich commented 3 years ago

Ah, yes. That was ideologically derived from computed behavior in other libs, e.g:

Vue:

computed properties are cached based on their reactive dependencies. A computed property will only re-evaluate when some of its reactive dependencies have changed

MobX:

Computed values can be optimized away in many cases by MobX as they are assumed to be pure. For example, a computed property won't re-run if none of the data used in the previous computation changed.

I think that's something that is expected from the name "computed", therefore I made that to be a default under-the-hood behavior.

While indeed Rx is a different beast and sometimes we need to cover all emissions. So, what do you think if we were to provide another API endpoint or a setting to allow two behaviors? (another naming challenge)

Jopie64 commented 3 years ago

Hmmm... For Rx it indeed doesn't feel that obvious though 🤔 But it might be what users want... What about an API to be able to set some configuration? So one can have their own computed method like this:

import { configure } from 'rxjs-autorun';

export const computed = configure({
  defaultStrength: 'strong',
  ignoreSameValue: false
});

Something like that...

rxjs-autorun could still expose a computed as well, with sane defaults.

kosich commented 3 years ago

~That is cool if you're intending only one scenario per project. Though you want two behaviors and used in two different places of one project — the configure(…) might switch back n forth globally, which would lead to unexpected results.~

As much as I don't like blowing up our not so small dictionary, maybe another verb / adjective would fit? Something like (but not exactly) effect(() => … ) react(() => … ) expression(() => …) derive(() => …)

UPD: Totally missed you're redefining it, thought it was reconfiguring. Sorry! Thinking...

kosich commented 3 years ago

I still tend towards a separate name:

Jopie64 commented 3 years ago

Separate names would be cool too I think. Especially for the reasons you mention indeed! We could still expose a configure (or something like configureComputed) as well too and use it internally to create the other names.

export react = configureComputed({
  ignoreSameValue: false,
  ...
});

Best of both worlds?

kosich commented 3 years ago

For internal use — sure, for external use — mm, I think when we have something more then a single flag 🙂

And, IMHO, generally the simpler API is — the better (though looks like we already fail this test 😄)

Back to the names: got anything better than derive? 😅 I'd be glad to hear more options :)

Jopie64 commented 3 years ago

Simpler is better, yea, I agree!

Actually I'm not sure what you mean by these names... I'd figure, with react you mean something like, do things on every change even when there's no difference (hence no distinctUntilChanged) and with derive it's just about the value and only signal when it's changed or something? (Hence with distinctUntilChanged?) I'm not sure about the other two... Differences in default binding strength?

kosich commented 3 years ago

:D

I meant we keep computed with distinctUntilChanged and come up with a second name for a function what wont use distinctUntilChanged nor on inputs nor on output.

(do we agree on this?)

...aand for that function I'm seeking a name. I wrote those options only as possible candidates for this single function.

UPD: agree, derive is synonymous to computed. probably a bad name. skip it :)

Jopie64 commented 3 years ago

Ah that way! I misunderstood :D I'm terrible with names... I wish they could scrap that from our job :D Can't come up with one right now... (computed2? ;P ) I'll think about it.

kosich commented 3 years ago

I'm still empty... maybe combine like combineLatest (or combined — similar to computed)?

What about re-purposing autorun? (not so sure about this)

Other so-so option: fromExpression, …

Jopie64 commented 3 years ago

Sorry bit busy lately :) On vacation next week, so pressure from my job this week... Not much here either... computeAll? Maybe pause this and first see how it's used... See what is expected when it's actually used.

kosich commented 3 years ago

No worries! Have a good inspirational rest! 🙂

loreanvictor commented 3 years ago

My 2 cents on the matter: I do realize that by default not re-emitting redundant values can be pretty sweet and desired behavior in most cases, however if we were to exclude re-emissions due to emission of a source that is then not utilized during the last run, you wouldn't be getting too many extra emissions without using distinctUntilChanged as well.

The current implementation does however bar using notifier sources or notification expressions, where there is no value of interest and the expression is designed to control notification rate:

() => $(slowMotion) ? $(slowTimer) : $(fastTimer)

I am not 100% sure of actual usefulness of such expressions to be honest, but I feel it would be a shamed if using this utility in such creative ways was not possible.

As for adding another function: Seems a bit strange to me tbh, since enabling this is quite easy (.pipe(distinctUntilChanged())) and seems unnecessary to increase API surface for such a thing. However, if that is the chosen approach in the end, I would like to suggest naming the rawer function expr().

kosich commented 3 years ago

I agree that we need something undistilled for notifications. And I think we'll include this fn in the next version.

As to distinctUntilChanged — it derives from the "computed" term that seems to be widespread in reactive libraries (as mentioned above). Consider computed as a dynamic value, rather than a stream. So it emits only when it's result changes, and is recalculated only when it's dependants change (the latter is harder to manually apply via distinctUntilChanged).

Obviously, this version of "computed" in Rx is not equal to those in MobX or Vue, but it's close enough. And I think it's good to have this API out-of-the-box, as it might be handy and familiar to some Rx newcomers.

--

Yet, when we add these new "raw" expressions, seems like "autorun" fn should derive from that. As the name is closer to "react to every notification" than to "calculate this on distinct updates". What do you think, guys?