Moving to an API with AbortSignal

benlesh commented 4 years ago

Since it seems like Node is now also landing AbortSignal, a proposal around creating a standard cancellation semantic should be prioritized (probably over this proposal). The reactive programming community would benefit greatly from that.

That said, if it does end up being in-line with AbortController/AbortSignal. I'd propose the API for the Observable proposal reflect that:

(Note the absence of the Subscription type)

interface Observer<T> {
  next(value: T): void;
  error(err: any): void;
  complete(): void;
}

interface Observable<T> {
  forEach(nextCallback: (value: T) => void, signal?: AbortSignal): Promise<void>;
  subscribe(observer?: Partial<Observer<T>>, signal?: AbortSignal): void;
}

Related thread in RxJS here: https://github.com/ReactiveX/rxjs/issues/5545

benlesh commented 4 years ago

FWIW: I'm not super enthused about the name "abort", or the use of EventTarget (addEventListener, etc), but I think this is a solid move to make something that is coherent with the rest of the ecosystem.

benlesh commented 4 years ago

cc @zenparsing @jhusain

ljharb commented 4 years ago

It's very much an unfortunate name, and I don't think it would be appropriate to enshrine it further (and i think it's a bad idea for node to land it - and it's still experimental in node)

benlesh commented 4 years ago

Sad though it is, I'd still rather cancellation standardize around something. And unfortunately, the AbortController ship has sailed, I think. I'd love to hear theories about how to move to something better though.

josepot commented 4 years ago

It's very much an unfortunate name

@ljharb could you please elaborate a little bit more on this? I'm worried that maybe I'm not understanding your concerns due to the fact that English is not my first language. At first I thought that perhaps it was because the term "abort" had some sort of "insensitive" or "inconsidered" connotations and maybe I wasn't aware of that. So, I ran it through alexjs.com and it didn't give me any warnings. So, now I'm really wondering why do you find it to be such an unfortunate name?

i think it's a bad idea for node to land it

Do you think that it's a bad idea that they land it under that name, or you also have reservations with the behavior/ergonomics of the API?

I'm sure that I must be missing a lot of context, so please forgive me if what I'm about to say is not accurate.

The only thing that worries me about @benlesh idea is the fact that currently the AbortSignal API is part of the DOM spec. I'm of the opinion that it makes a lot of sense for NodeJS to adopt it, because from the point of view of Node it's ok to borrow APIs from the DOM (e.g: setTimeout, setInterval).

However, I think that if we want for the Observable proposal to work with AbortSignal (FTR: I think that would be awesome!), then I suppose that would require to first submit a proposal-abort-signal (or something with a different name but a similar behavior) to TC39?

benjamingr commented 4 years ago

It's very much an unfortunate name, and I don't think it would be appropriate to enshrine it further (and i think it's a bad idea for node to land it - and it's still experimental in node)

Fwiw, as far as I'm aware that's a concern no one championed in Node.js (in the issue or the summit) as far as I know - we can happily go to WHATWG and suggest a rename to a different name (and follow that in node). I am not even sure they would object to having an alias with AbortController/AbortSignal being supported for backwards compat.

I don't think (I might be mis-remembering) anyone felt particularly strongly about the name - I it was called AbortController because that has historically been the terminology in other languages (like c#'s HttpRequest.Abort, Java's apache HttpGet.abort, ECONNABORTED in posix etc).

Here is some of that discussion (you even participated 😅 ) https://github.com/whatwg/dom/pull/434#issuecomment-292696490 I think if this bothers you (personally I am rather ignorant on why the naming is problematic) I would:

Raise this with whatwg/dom with a concrete suggestion (naming it CancelationSignal like the PR originally did - support the old naming, explain why the change).
Gain consensus there and make the spec change (not a ton of work).

I strongly feel that Node should adopt the existing standard rather than make up a new standard, I am happy to rename the version Node.js ships to the new name once it gets whatwg support.

benjamingr commented 4 years ago

@josepot

Do you think that it's a bad idea that they land it under that name, or you also have reservations with the behavior/ergonomics of the API?

Fwiw, there are a lot of concerns that we worked through when landing the API in Node.js (mostly related to EventTarget and Error semantics). I can happily link to the summit discussion and James' PR if you want more info.

The only thing that worries me about @benlesh idea is the fact that currently the AbortSignal API is part of the DOM spec. I'm of the opinion that it makes a lot of sense for NodeJS to adopt it, because from the point of view of Node it's ok to borrow APIs from the DOM (e.g: setTimeout, setInterval).

Node does not implement the timer spec (web timers) and its timers are not spec compliant and behave differently from web timers (both in edge cases and in "obvious" stuff - for example the return value of setInterval is a number in browsers but an object in Node, browser timers can take a string and Node's can't etc).

Node supporting AbortController is about there being (one) supported unified way to cancel asynchronous actions in code. This means I can for example convert my promises code to RxJS or my async iterator code to RxJS and not have to change the way cancellation works throughout. It also lets me mix-and-match RxJS code (as an example) in a non-Rx code base in a clean-ish way.

However, I think that if we want for the Observable proposal to work with AbortSignal (FTR: I think that would be awesome!),

Honestly I'm not sure how that would work. Ben is a lot more up-to-date on where that proposal stands than me but if I was related I'd focus on adoption in platforms and promote the standardisation efforts through there. As far as I understand (from Daniel) cancellation in JS is going to be "AbortController compatible" (although all of this is stage 1 and susceptible to change).

josepot commented 4 years ago

@benjamingr

I can happily link to the summit discussion and James' PR if you want more info.

Yes, please. If it's not going to take too much of your time, I would be very interested. Thank you!

Node does not implement the timer spec (web timers) and its timers are not spec compliant and behave differently from web timers (both in edge cases and in "obvious" stuff - for example the return value of setInterval is a number in browsers but an object in Node, browser timers can take a string and Node's can't etc).

Sorry, I guess that I didn't explain myself very well. I knew that. What I meant to say is that Node is more than ECMAScript. At least in the sense that Node's "goal" was to bring to the server the same way of leveraging JS that we got in the web, and for doing that it put the event-loop at the forefront (I could be wrong, but I think that ECMAScript has no references to the event-loop... And yes, I know that these days Node also supports Worker Threads :slightly_smiling_face:). But hopefully you get what I'm trying to say. At least in the way that I see it: Node is more than ECMAScript and it "takes inspiration" from the Web, and that's ok, it's its nature.

Node supporting AbortController is about there being (one) supported unified way to cancel asynchronous actions in code. This means I can for example convert my promises code to RxJS or my async iterator code to RxJS and not have to change the way cancellation works throughout. It also lets me mix-and-match RxJS code (as an example) in a non-Rx code base in a clean-ish way.

I think that this is amazing :heart_eyes: and I'm very curious to learn more about it. In fact, I think that this is so incredibly awesome that I hope that TC39 tries to find a way to bring ~this~ a unified way to cancel async actions to ECMAScript :pray:

Ups! I just re-read @benlesh first message and it seems that this is exactly what he was asking for :facepalm::

a proposal around creating a standard cancellation semantic should be prioritized (probably over this proposal)

I'm so sorry... I read the title, I looked at the code-snipped and I guess that I skipped the most important part. I'm so sorry. I will see myself out of here, and will let you continue the conversation. Sorry for the noise.

benjamingr commented 4 years ago

Yes, please. If it's not going to take too much of your time, I would be very interested. Thank you!

https://docs.google.com/document/d/19lEj6h1xDe1I2lEvW_HSoyYG1OfAJO2PLe4dNE7gJ0w/edit https://github.com/openjs-foundation/summit/issues/273

tc39 / proposal-observable

Moving to an API with AbortSignal #209