getify / CAF

Cancelable Async Flows (CAF)
MIT License
1.34k stars 46 forks source link

Switch cancelToken to AbortSignal #2

Closed Haroenv closed 6 years ago

Haroenv commented 6 years ago

As in fetch:

const controller = new AbortController;
const signal = controller.signal;

// pass it around
async function cancelable(signal, ...rest) {
  signal.addEventListener('abort', () => {
    /* stop listening */
    throw new AbortError('was canceled');
  });
  // rest of the implementation
}

controller.abort();

See also https://developer.mozilla.org/en-US/docs/Web/API/AbortController

So in practice, we should probably accept both an AbortSignal (possibly polyfilled: https://yarn.pm/abortcontroller-polyfill) and the cancelToken

getify commented 6 years ago

Since we need to support node (not sure if AbortController is in node?), was thinking the cancelToken interface could just change to match, but still be there (to sort of act as a simple polyfill of sorts).

It would be ideal if fetch(..) could receive the cancelToken and have it "just work", as the design goal with CAF is to make the cascading of cancelation easy.

getify commented 6 years ago

also, another important detail (for consistency): AbortError is the promise rejection upon cancelation.

Haroenv commented 6 years ago

AbortController is not yet in node (no plans either AFAICT) and not in browsers except preview and very latest version. It does seem like the way to go though. The polyfill I linked works in node+browser though, so that might be an option.

getify commented 6 years ago

The AbortController polyfill (only, not the fetch patch) seems fine to just bundle in the util, in place of CAF.cancelToken.

getify commented 6 years ago

Some downsides to switching to just using AbortController instead of a custom cancelation token:

  1. abort() doesn't take any abortion reason message to pass along (as the promise rejection), which can be very useful in certain circumstances.

  2. It would be really nice if there was a way to vend a promise (that will be rejected) from a cancelation token, either as the return value of the listen(..) (aka abort()) call, or just as a pr property on the token object itself.

    The use case for that is when you want to manually listen to the signal, because you're calling off to a function that doesn't understand cancelation signals.

    var main = CAF(main);
    var cancel = new CAF.cancelToken();
    main(cancel,"http://some.tld");
    
    function *main(cancelToken,url) {
    
      var resp = yield Promise.race([
         cancelToken.listen(),   // vend a promise that rejects on cancelation
         ajax(url)   // utility that doesn't understand cancelation tokens
      ]);
    
      console.log(resp);   // only get here if not canceled first
    }

This second one is a particular concern for me, as it's an important feature I wanted to add to CAF. The first one is already a feature, and would be lost if we switched.

Trying to think if there's a clean way to style the CAF.cancelToken to be compatible with AbortController, but sort of extended. Thoughts welcomed.

getify commented 6 years ago

The polyfill for AbortController does not work in node, as it relies on using document which is only present in the browser. :(

Haroenv commented 6 years ago

https://github.com/mysticatea/abort-controller

getify commented 6 years ago

That one has a wonky implementation (using WeakMap).

I decided to just modify the implementation from the other one a little bit, enough to get it to work.

getify commented 6 years ago

So what I ended up with is CAF.cancelToken() is a sub class of AbortController, and it extends it with the behavior I described a few comments earlier in this thread. You access the promise for the signal with signal.pr instead of calling a listen() method. token.abort(..) takes a reason value and rejects the promise with it.

getify commented 6 years ago

Unfortunately, what I initially did to extend the AbortController and AbortSignal doesn't work (when tested in FF59), because of weirdness with how those are defined in the HTML spec --- both are actively hostile to extension, for some crazy reason. I had to find work-arounds, and that's now in 3.0.0.