ericelliott / speculation

JavaScript promises are made to be broken. Speculations are cancellable promises.
MIT License
197 stars 9 forks source link

Errors thrown from onCancel() are swallowed. #4

Closed ericelliott closed 7 years ago

ericelliott commented 7 years ago

An easy fix is to catch them, e.g.:

const speculation = (
  fn, cancel = Promise.reject('Cancelled')
) => new Promise((resolve, reject) => {

  const handleCancel = (
    onCancel
  ) => cancel.then(onCancel).catch(e => {
    if (e.message !== 'Cancelled') reject(e);
  });

  return fn(resolve, reject, handleCancel);
});

But if you look closely, you'll notice that we need to be able to distinguish any random error thrown in onCancel() from the intentional Cancelled error. To do so, we'll need to clearly identify Cancelled errors. We could do that with a custom error type, or we could use the error message, as above.

It would be nice if users could pass a custom message, but I also don't want to be using instanceof. (Will we misidentify things if the custom error type is created in a different execution context?)

ericelliott commented 7 years ago

Another possible solution is to handle the normal cancel error with a noop and then catch the success handler's error with .catch():

const speculation = (
  fn, cancel = Promise.reject('Cancelled')
) => new Promise((resolve, reject) => {
  const noop = () => {};

  const handleCancel = (
    onCancel
  ) => cancel.then(
    onCancel,
    noop
  ).catch(e => reject(e));

  return fn(resolve, reject, handleCancel);
});

That said, I still think we should supply an easily identifiable cancellation error that users can use in their handlers, but we can deal with that elsewhere.

atticoos commented 7 years ago

Perhaps a custom error type?

export const CancellationError = ...

const speculation = (
  fn, cancel = new Promise.reject(new CancellationError())
) ...

Where it would be easy to catch on the type

import speculation, {CancellationError} from 'speculation';

const wait = ...

wait(200, wait(50))
  .then(() => console.log('Yerp'))
  .catch(e => {
    if (e instanceof CancellationError) ...
    else ...
  })

And much simpler with bluebird:

.catch(CancellationError, () => console.log('Cancelled'))
.catch(e => console.log('failed', e))
ericelliott commented 7 years ago

instanceof lies when you try to use it across execution contexts.

atticoos commented 7 years ago

Wow, good call.

Had an interesting read following up on this via https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/instanceof#instanceof_and_multiple_context_(e.g._frames_or_windows).