search

kosich / rxjs-autorun

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

Updates w/o distinctUntilChanged #32

Closed kosich closed 3 years ago

kosich commented 3 years ago

fix #21. It's based on #31, and is waiting for it to resolve.

I chose to use combined name, which seems to be familiar to RxJS devs with combineLatest and reflect it's purpose well: combine values from multiple streams.

kosich commented 3 years ago

Hey, @loreanvictor ! I've noticed that you've forked the repo. I'm going to merge this and #31 today (if you don't have objections) So please plan your work accordingly.

Sounds ok?

loreanvictor commented 3 years ago

Hey, @loreanvictor ! I've noticed that you've forked the repo. I'm going to merge this and #31 today (if you don't have objections) So please plan your work accordingly.

Sounds ok?

Yep! I forked the repo to do some bundler checks next week (alongside some memory consumption tests, etc), so all is good. And this change is much appreciated, however may I ask why the option is an enum instead of, for example, an array of pipeable operators that defaults to [distinctUntilChanged()]?

Jopie64 commented 3 years ago

I also think this is a good change! Nice that you came up with this combined name, I think that's a good one!

As of the suggestion to use an array of pipable operators: I'd not be in favor of that, because you cannot know what type of values will run through them... That's also one of the reasons why I proposed to not use distinctUntilChanged (#21): it could be that the values are not comparable by the default comparer of it, so it would do unnecessary checks in that case.

kosich commented 3 years ago

Yep! I forked the repo to do some bundler checks next week

@loreanvictor awesome! Looking forward to that!

As to an array of operators: good point. I guess since we currently have only two states (distinct or not), I wanted to be verbose about that. Though you might be right about this 🤔

@Jopie64 , I didn't think of indistinguishable values. Yeah, users could use this combined fn in those cases. Not sure if we need bigger flexibility, it's an interesting issue.

Regarding array vs enum — I think @loreanvictor meant inner implementation, not the external API. So the end-user gets equal behavior either way.

loreanvictor commented 3 years ago

I think @loreanvictor meant inner implementation, not the external API. So the end-user gets equal behavior either way.

Pretty much, the idea is to have a more future-proof/flexible internal API. For external usage, I suspect the most common use cases seems to be distinct or not, and for more than that well people can pipe incoming/outgoing observables to whatever operators they need manually and exposing that ability to provide an array of operators wouldn't provide much further convenience on that front (actually makes the code harder to read).

kosich commented 3 years ago

@loreanvictor , I looked at enum -vs- array thing again today. The array approach required some tweaks for types (should that array of operators produce the same type Observable?). While the enum approach was too verbose 😕

So I decided to go with a minimal boolean flag for now and extend it when we have a better vision on the next API. Ok?

loreanvictor commented 3 years ago

So I decided to go with a minimal boolean flag for now and extend it when we have a better vision on the next API. Ok?

Yep sounds perfect for the time being. The rest we should decide basically based on usage rather than guess-work.

kosich commented 3 years ago

FYI, guys, released this and a couple other PRs with rxjs-autorun@0.0.2! 🙌 Thank you for your help! 🙏