WICG / observable

Observable API proposal
https://wicg.github.io/observable/
Other
546 stars 12 forks source link

`subscribe(fn)` and/or `subscribe(observer)` #71

Closed benlesh closed 7 months ago

benlesh commented 9 months ago

Most of the observable use in the wild boils down to someObservable.subscribe(function), but someObservable.subscribe(observer) is necessary for a variety of reasons. While most of the time people don't care about error handlers and complete handlers, they're still important. Having one object that can carry those things is beneficial for things like shared observation.

If we wanted to support either a function (the most common path) or an observer, we could use this pattern:

source.subscribe(console.log, { signal });
source.subscribe({
  next: console.log,
  error: console.error,
  complete: () => console.log('done'),
}, { signal });

The above pattern aligns very well with all known Observable implementations in the wild. As the majority of them take only one argument, which is usually a function.

Prior art for this is on EventTarget itself, where both of these work:

button.addEventListener('click', (e) => console.log('clicked'))
button.addEventListener('click', {
  handleEvent() {
    console.log('clicked');
  }
})

However, I realize there's an argument to be made that forEach accomplishes the goal for subscription with just a function. Just as a principal, I feel like mixing next, error, complete and signal might be mixing concerns in a way that isn't beneficial. Also subscribe(fn) is still the most common path for observable subscription in user-land, as it's more ergonomic and it doesn't allocate a promise that no one will use (ala forEach).

Related to shared observation, having the signal argument on the Observer is a bit problematic. Here's an example why:

class LoggingObserver {
  #totalMessages = 0;

  log(type, value) {
    this.#totalMessages++;
    const messageNumber = this.#totalMessages;
    console.log(`(${messageNumber}) ${type}: ${value}`)
  }

  next(value) {
    this.log('next', value);
  }

  error(error) {
    this.log('error', error);
  }

  complete() {
    this.log('complete');
  }
}

const observer = new LoggingObserver();

// This will NOT work because the handlers aren't bound properly
const ac1 = new AbortController();
source1.subscribe({ ...observer, signal: ac1.signal });
const ac2 = new AbortController();
source2.subscribe({ ...observer, signal: ac2.signal });

// A design like this would work fine.
const ac1 = new AbortController();
source1.subscribe(observer, { signal: ac1.signal });
const ac2 = new AbortController();
source2.subscribe(observer, { signal: ac2.signal });
petamoriken commented 8 months ago

Make sense for me. I think it would be better to pass the AbortSignal as a separate option to receive EventTarget#addEventListener as well.

callback dictionary Observer {
  ObserverCallback next;
  VoidFunction complete;
  ObserverCallback error;
};

dictionary ObserverOptions {
  AbortSignal signal;
};

[Exposed=*]
partial interface Observable {
  undefined subscribe(optional Observer observer = {}, optional ObserverOptions options = {});
};
domfarolino commented 8 months ago

So there seems to be two issues wrapped up here:

  1. Allowing subscribe(fn) and subscribe(observer)
  2. Separating signal from Observer (this should probably be a separate issue)

I'm sympathetic to the reasoning for (1), and the desire to maintain compat between native Observables and userland Observables. However note that WebIDL does not allow this today. The addEventListener() example works (i.e., allows a function or dictionary to be passed in as the event listener) because EventListener is defined as a callback interface (as opposed to a callback function), and callback interfaces must have "exactly one regular operation", whereas Observables allow for up to three operations to appear on an Observer dictionary.

Furthermore, the usage of callback interface appears to be very discouraged. So we'd need a compelling reason to use it anyways, and a compelling reason to extend it to allow more than one regular operation. I don't know enough about the governance of Web IDL to know how big an ask that is, but I'd love to hear from @annevk on this.

bakkot commented 8 months ago

I hope technical restrictions of how webIDL currently works don't affect the design of this proposal - needs of authors are supposed to outweigh those of implementors, and definitely outweigh those of spec authors.

If the design of (1) is bad for other reasons, that's fine, but if we'd go with (1) except for webIDL limitations, that would be bad.

domfarolino commented 8 months ago

Yes, to be clear I totally agree! I just want to know if there are super good design reasons (that have user interests in mind, not technical purity) why we really shouldn't change WebIDL to support this / why callback interface should not be more widely used.

domenic commented 8 months ago

callback interface is bad, but allowing CallbackType or DictionaryType is reasonable, and is just waiting on a second implementer (beyond Chromium) to support it: https://github.com/whatwg/webidl/pull/1353

annevk commented 8 months ago

When I looked at this a while back the issue Ben raises around coupling of signal seemed the most significant to me, but I guess there's a desire to discuss that separately. Was an issue opened?

If you use a dictionary, would this.log() as used in Ben's example actually work? Looking at https://streams.spec.whatwg.org/#ws-constructor it wouldn't and you would need to use CallbackType or object (which doesn't work), making it essentially a callback interface by disguise. What is the difference at that point? Maybe it's not bad when there's multiple members?

domfarolino commented 8 months ago

When I looked at this a while back the issue Ben raises around coupling of signal seemed the most significant to me, but I guess there's a desire to discuss that separately. Was an issue opened?

I've been hearing this as well. It sounds important to the community, but I think it's just different from the issue of whether or not the first argument passed into subscribe() can be a function-or-a-dictionary. Whether or not that dictionary should contain the signal (or if another dictionary should contain the signal) just seems like a different issue altogether. But no I don't think one has been filed. @benlesh would you be able to do that? You seem to know more about why that is necessary, whereas my non-JS-developer mind has trouble remembering why separating the callback dictionaries from the signal/options dictionary is important.


callback interface is bad,

Just for my own learning, why is it bad?

but allowing CallbackType or DictionaryType is reasonable, and is just waiting on a second implementer (beyond Chromium) to support it: https://github.com/whatwg/webidl/pull/1353

Wow, this is huge. That seems to be exactly what we're looking for here, thanks!


Regarding @annevk's comment: I'm not actually sure if that example would work? Converting to a dictionary would pluck off the relevant methods from the object, but I guess you're saying the this binding inside of those methods would be broken? I'm actually not sure how that works, to be honest. I guess I'd expect this to close over the relevant object, so that the methods would just work fine on that object, but maybe not.

What is the difference at that point? Maybe it's not bad when there's multiple members?

Sorry just so I'm clear, are you drawing attn to the fact that there might be no difference between CallbackType or DictionaryType and just using callback interface-with-multiple-members?

domenic commented 8 months ago

CallbackType or object (which doesn't work)

I think we could make it work, in the same way we're proposing to do for dictionaries. But, see https://github.com/WICG/observable/issues/22#issuecomment-1804938312 for why it might not be good anyway? Basically I'm unclear whether encouraging per-observable state (as opposed to per-subscriber state) is a good idea, or a footgun.

making it essentially a callback interface by disguise. What is the difference at that point?

Just for my own learning, why is it bad?

Basically callback interfaces are not designed for what we're talking about here. They're just legacy things to support the weird behavior of addEventListener() where it can sometimes take a { handleEvent() { ... } } object. For example:

We could define a new thing which works well for this case, Streams, and possibly custom element callbacks as well (although that case is a bit more complicated because you want to take a constructor/class, and disallow plain object literals: https://github.com/whatwg/webidl/issues/701). I think maybe worklets would benefit too.

Sorry just so I'm clear, are you drawing attn to the fact that there might be no difference between CallbackType or DictionaryType and just using callback interface-with-multiple-members?

@annevk is contrasting something like CallbackType or object, and then processing the object by converting it to a dictionary and later invoking methods extracted from the dictionary, with callback interface-with-multiple-members. The former pattern is what Streams is doing today; see e.g. https://streams.spec.whatwg.org/#ws-constructor for the conversion and then https://streams.spec.whatwg.org/#set-up-writable-stream-default-controller-from-underlying-sink for the extraction/invoking.

domfarolino commented 8 months ago

Thanks for the detailed explanation. I think it all makes sense, except I'm confused about this one thing:

Using them from specs requiring using "call a user object's operation" with an explicit thisArg, instead of doing what you'd want for this case which is to automatically save the object and always use it as the thisArg.

It is indeed obvious that we'd want to use the passed-in-object object as the thisArg for all method invocations? https://streams.spec.whatwg.org/#set-up-writable-stream-default-controller-from-underlying-sink seems to do this by using the original object passed in as the callback this value. This seems interesting, but it kinda seems like it could go either way and we just have to choose. But I'm curious if there's some precedent really putting pressure one way or another, or if the decision really just hinges on whether we want to introduce a new surface area for per-Observable state, as opposed to the per-Subscription state that we already get with the Subscriber interface.

domenic commented 8 months ago

This seems interesting, but it kinda seems like it could go either way and we just have to choose. But I'm curious if there's some precedent really putting pressure one way or another

Most cases do not pass a thisArg, because most cases on the platform use dictionaries. In certain special cases, we recognize that the object being passed in could usefully be an instance of a class, or want to share state among its functions, and so those functions are really "methods" and not "functions inside a dictionary". In those special cases we call with a thisArg.

Much of the discussion here and in chat is about making it easier to write specs for those special cases.

Overall, I would default to treating them as "functions inside a dictionary" until a compelling argument appears otherwise. I thought we had such an argument, but it seems I was wrong.