search

kosich / rxjs-autorun

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

Refactoring #28

Closed kosich closed 3 years ago

kosich commented 3 years ago

This PR doesn't remove/change existing tests ~Mainly it moves things around~ ~A bigger change is creating a separate stream for errors~ It removes intermediate Subject and pushes directly to the observer. It also fixes some minor issues (runner fn type, exporting error)

kosich commented 3 years ago

Commit 194b42f switches to direct pushing events to the observer, suggested by @loreanvictor (thx!) With this change, we can drop Subject and almost all operators (well, I left distinctUntilChanged)

All the tests are passing, but I haven't yet checked everything thoroughly.

loreanvictor commented 3 years ago

I have a (rather weak) argument against using distinctUntilChanged as well. I do realize that if a tracked observable emits but its value is not utilized in re-evaluation then the result of re-evaluation should not emit (to be honest regardless of whether it has changed due to perhaps changes in passive observables or not), but that can be easily checked independently and besides that it would seem more natural to me to allow (actively tracked) observables emit the same value (for example, undefined) and have that result in the computed() to also re-emit. Simply because I am curious about potential expressions that mainly control flow of some notification emissions (so the actual value doesn't matter), specifically in cases like this:

() => f($(a)) ? $(b) : $(c)

where b and c are mere notifiers (so they always emit undefined).

kosich commented 3 years ago

@loreanvictor , not sure I got your point fully. We plan to have a separate API endpoint that won't use distinctUntilChanged not on input, nor on output. Would this help? Details in #21

loreanvictor commented 3 years ago

@kosich that's also an option, anyways then let's keep that discussion in that thread.

kosich commented 3 years ago

I'm going to double-check it and merge for the sake of moving forward @Jopie64 please, read it through post-mortem when you're available 🙂

kosich commented 3 years ago

@loreanvictor , I'd be glad if you gave it a look too 🙂

loreanvictor commented 3 years ago

@loreanvictor , I'd be glad if you gave it a look too 🙂

sure thing! will take a look when I get home.

loreanvictor commented 3 years ago

@kosich looks good (and much more readable, kudos).