kosich / rxjs-autorun

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

A way to suppress/skip an emission #19

Closed kosich closed 3 years ago

kosich commented 3 years ago

I was about to discuss if we need a way to skip (suppress, filter) an emission based on some logic:

import { compute, $, _, SKIP } from 'rxjs-autorun';

const even$ = compute(() => $(o) % 2 == 0 ? _(o) : SKIP);

SKIP being some special object that we'll filter out.

But then again I realized that the magic is already there:

import { compute, $, _, SKIP } from 'rxjs-autorun';
import { NEVER } from 'rxjs';

const even$ = compute(() => $(o) % 2 == 0 ? _(o) : _(NEVER));

Is not that beautiful? Think we gotta document this trick too 🙂

cc @Jopie64

Jopie64 commented 3 years ago

Whaha nice find! 👍😁 Making use of late emission.

It makes me wonder what would happen with EMPTY

kosich commented 3 years ago

checked: the same, just probably with additional check for completion

This is the second finding (with the $($(x)) thing) in a quite small lib that we didn't intend. 🧐

Jopie64 commented 3 years ago

I was thinking, with this trick it also would have been nice when weak binding was the default ðŸĪŠ

const r = computed(() => {
   switch($(a)) {
      case 0: return $(b);
      case 1: return $(c);
      default: return $(NEVER);
   });

Also wondering what typescript will make of type r. ðŸĪ” I'm guessing it will still be a union of a and b, cause something | never will be something.

kosich commented 3 years ago

😄 I still stay strong to these weak attacks, but you make a good point:

compute(() => $(a) ? $(b) : _(EMPTY));

If we use EMPTY that completes, would that clean-up normal b?

--

TS: yeah, it seem to handle the type nicely (at least with the Elvis operator ?: ) ).

Jopie64 commented 3 years ago

Guessing EMPTY wouldn't help cause it can never emit a value hence it will still do a midflight interrupt there...

We could however be sure that it will never be able to change cause it completed too. So we could make an exception that even in 'normal' binding it will unsubscribe when interrupted by a completed observable? 😇

Yea I keep trying 😜

kosich commented 3 years ago

:D

Well, I meant exactly that! Since EMPTY has completed — we can probably do the cleanup. And it makes sense.

Although we gotta better think through such cases of empty observable completion, e.g.:

computed(() => $(a) + $(EMPTY));

What is the expected behavior here? Will it effectively become NEVER now?

Jopie64 commented 3 years ago

Good example! Not sure why such a scenarios would be different from the current version when you were just using weak binding though... This would indeed effectively yield NEVER (when a doesn't complete), cause a is still used here so it will not complete. (According to the runtime, a different value of a might result in a successful run of the expression.)

kosich commented 3 years ago

The issue is that we wont be able to distinguish $(a) ? $(b) : _(EMPTY) from $(a) + _(EMPTY) Thus expression that won't resolve by definition will keep hanging till a completes (or expression unsubscribed) Think we gotta document this too 😔

Jopie64 commented 3 years ago

You are correct that these expressions can't be distinguished at runtime, and you can document that if you want... But I still don't see why this would only arise if we threat normal binding like weak binding if it interrupted due to completion...? These cases already existed right? They would not act any different no matter which binding you use. I also think they are more or less covered by the completion tests...

kosich commented 3 years ago

No-no, I think this issue exists regardless of subscription or tracking type.

I didn't mean to attack weak tracking 😅

Though we might want to be more strict and throw in such cases:

  1. it really indicates an error that expression is possibly un-computable
  2. users would still be able to handle it precisely with defaultIfEmpty
  3. for filtering sake users will use NEVER not EMPTY
Jopie64 commented 3 years ago

So... as I understand correctly... you are willing to throw when it encounters an observable that completes before it emits? I'm actually not so sure about that... When I come back to this case:

const r = computed(() => {
   switch($(a)) {
      case 0: return $(b);
      case 1: return $(c);
      default: return $(EMPTY);
   });

(But now use EMPTY instead of NEVER) then it will throw when $(a) is not 0 or 1? But if instead we use NEVER it will not unsubscribe from the observable from previous case (in normal mode)

I'm of the opinion that we should just make the specs simple and clear, and don't 'protect' the user by trying to detect mistakes and add some complicated specs to do so. Experience tells us that these less simple things will bite you later :)

kosich commented 3 years ago

In case I confused you with "throwing" — I meant throwing to the output stream: observable.error(â€Ķ), not the mid-flight interruption.

Well, now I'm considering maybe it should complete instead of error to the output stream.

The reason for this thinking is that user may not know if their Observable will emit or not. Imagine a case where a and b are taken from outside world:

const a = callSomeAPI();
const b = callSomeOtherAPI();
const c = computed(() => {
  return {
    a: $(a),
    b: $(b)
  };
});

Now, if b is effectively EMPTY — then c won't ever emit, complete or error. Which would be strange. Additionally, there's no easy way for user to tell that this happened!

So I'm not protecting users from themselves, rather reporting to them impossible cases. It'd be bad practice if we swallowed erred expressions.

While in the case of filtering user can explicitly and quite easily instruct the API to eagerly unsubscribe from b & c when it can via $.weak, and to skip emission at all if a is neither 0 nor 1:

const r = computed(() => {
   switch($(a)) {
      case 0: return $.weak(b);
      case 1: return $.weak(c);
      default: return _(NEVER);
   });

And the spec for that would be easy: computed(() => $(a) + $(EMPTY)) should immediately complete.

Need to think of this a bit more. ðŸĪ•

If you have additional thoughts/arguments — please, share!

kosich commented 3 years ago

I've created a separate issue to discuss this case ( #22 ) as this was supposed to be documentation-related and was closed with #20. (I ended up documenting filtering via NEVER because it felt to be safer. we might choose to edit it back, depending on #22)

Please, feel free to reopen