alkemics / cancelablepromise Goto Github PK

I think have found inconsistency between ES Promise and Cancellable promise

describe('ESPromise', function() {
    it("should be resolved on first await", async function() {
        let promiseResolved = false;
        (async() => {
            await new Promise((resolve, reject) => {
                this.resolve = resolve;
            });
            promiseResolved = true;
        })();
        this.resolve();
        await (async() => {})();
        assert.isTrue(promiseResolved)
    });
});

describe('CancelablePromise', function() {
    it("should be resolved on first await", async function() {
        let promiseResolved = false;
        (async() => {
            await new CancelablePromise((resolve, reject) => {
                this.resolve = resolve;
            });
            promiseResolved = true;
        })();
        this.resolve();
        await (async() => {})();
        assert.isFalse(promiseResolved)
    });
});`

CancelablePromise is not resolved

Browser: Chrome 91.0.4472.124

I've noticed throwing error in wrapped (cancelable) promise don't forward it to wrapped promise.catch(...) block
Check example below:

import CancelablePromise, { cancelable } from 'cancelable-promise'

async func getSamplePromise(): Promise<any> {
   throw Error('Error that should be forwarded into promise.catch(...)')
}

let request: CancelablePromise<any> | null = null
const wrappedPromise = cancelable(getSamplePromise())

wrappedPromise.catch(err => {
   //that block is not going to be executed, and Error remains unhandled
})

When original Promise is used it enters .catch(...)

I have this test which uses async/await but it does not appear to cancel the promise

  it('promiseChain partial', async () => {
    let f = 0;
    const cp = cancelable(
      (async () => {
        await new Promise((r) => setTimeout(() => r(), 100));
        f = 1;
        await new Promise((r) => setTimeout(() => r(), 100));
        f = 2;
        await new Promise((r) => setTimeout(() => r(), 100));
        f = 3;
        await new Promise((r) => setTimeout(() => r(), 100));
        f = 4;
        expect(true).toBe(false);
      })()
    );
    await new Promise((r) => setTimeout(() => r(), 250));
    cp.cancel();
    expect(f).toBe(2);
  });

Maybe mention in the readme that AbortController is better to use. It can be natively used with fetch to abort a request and can also be used for other things. there is also a polyfill for it.

This code:

CancelablePromise.resolve(undefined).then(console.log)

doesn't log undefined whereas this code:

Promise.resolve(undefined).then(console.log)

does. Is it expected?

API suggestion

let promise = new Promise(...)
promise = CancelablePromise.from(promise);
promise.cancel();

In a JS project with "type": "module", in the package.json, the following code:

import { cancelable } from 'cancelable-promise';

will throw the error:

import { cancelable } from 'cancelable-promise';
         ^^^^^^^^^^
SyntaxError: Named export 'cancelable' not found. The requested module 'cancelable-promise' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'cancelable-promise';
const { cancelable } = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:132:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:214:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12)

Node.js v20.11.1

As mentioned in the error, you can do this as a workaround:

import CancelablePromise from 'cancelable-promise';
const { cancelable } = CancelablePromise ;

However this issue still leads to unexpected failures as the error can only be caught at runtime. Typescript does not catch this error at compile time.

Ultimately, this comes down to how the export is declared in the package.json. You can see the root of the problem here: https://publint.dev/[email protected]

I'll open a PR to fix this.

Tested with create-react-app here https://github.com/imnstr/cancelable-promise-issue
When CancelablePromise imported then i have message syntex error.
Library babel config preset with target ie11 exists, but dont works.
How to reproduce: start project via ### npm run start and go to ### http://localhost:8080/ using Google Chrome and IE11.
Expected: Hello title on the page and no errors.
Actual: Syntax error in console.

Hi, the document says that the finally callback will always be invoked event if the promise is canceled. But for the following snippet:

const {CancelablePromise} = require('cancelable-promise');
const promise = new CancelablePromise((resolve, reject) => {
  // Do something important...
}).finally(() => console.log('finally is called'));
setTimeout(() => {
  console.log('about to cancel');
  promise.cancel();
  // Just wait
  setTimeout(() => {}, 100000);
}, 1000);

, the finally is called is not printed.
Do I misunderstand anything?

I'm not sure if this is intentional or a bug, but here it is...

When I first started using this library I was saving my cancelable promises as such:

import CancelablePromise from 'cancelable-promise'
import Fetch from 'whatwg-fetch'

const getBlah = params => new CancelablePromise((resolve) => resolve(Fetch(...)))

let activeSearch = getBlah(...).then(response => ...)

In this case my activeSearch variable is still equal to an instance of CancelablePromise but calling activeSearch.cancel() before the fetch has resolved does not cancel the .then(response => ...) execution. After some frustration I tried something closer to what the README suggests:

let activeSearch = getBlah(...)

activeSearch.then(response => ...);

In this case calling activeSearch.cancel() does prevent execution of the pending .then().

It seems to me that any pending .then statements should be aborted no matter how you save your variable. However, I'm no expert with promises so not sure if this necessarily makes sense or is even possible given the current spec/polyfill.

It seems to me that removing from the last promise is more logical, what do you think?

cancel() {
  options.isCanceled = true;
  for (const callbacks of [options.onCancelList, options.finallyList]) {
    if (callbacks) {
      while (callbacks.length) {
        const onCancel = callbacks.pop();
        if (typeof onCancel === 'function') {
          onCancel();
        }
      }
    }
  }
},

It seems to me that instead of

executor(resolve, reject, (onCancel) => {
  internals.onCancelList.push(onCancel);
})

const promise = new CancelablePromise((resolve, reject, onCancel) => {
  onCancel(() => {
    //cancel code
  });
});

it is a bit easier to use such code

const onCancel = executor(resolve, reject);
internals.onCancelList.push(onCancel);

const promise = new CancelablePromise((resolve, reject) => {
  return () => {
    //cancel code
  };
});

Calling .then() on a CancelablePromise returns a Promise, which can't be canceled anymore.

Note: looking at the source .then() calls makeCancelable 🤔

There 1500 ms to run and promise3 must be canceled, but it is not.

Timeline:
0 ms: create promise
1000 ms: promise2 was created and finished, than create its children promise3
1500 ms: cancel promise2
2000 ms: promise3 was finished, but it shouldn't have happened

const {CancelablePromise} = require("cancelable-promise");

const promise = new CancelablePromise((resolve, reject, onCancel) => {
  const timer = setTimeout(() => {
    console.log("promise 1 was finished");
    resolve(); 
  }, 1000);
  const abort = () => {
    clearTimeout(timer);
  };
  onCancel(abort);
});

const promise2 = promise.then(() => {
  const promise3 = new CancelablePromise((resolve, reject, onCancel) => {
    const timer = setTimeout(() => {
      console.log("promise 3 was finished");
      resolve(); 
    }, 1000);
    const abort = () => {
      clearTimeout(timer);
    };
    onCancel(abort);
  });
  return promise3;
});

setTimeout(() => {
  promise2.cancel();
}, 1500);

At first glance, it is nice that you can use Promise.All and similar stuff, but this creates many independent threads that need to be processed. All you want after calling cancellation is to stop everything as possible and also stop the creation of new things like promises. I really advise you to remove this callback to avoid complicating the application logic and possible mistakes. At least write a warning in the documentation.

I tried the following (returning a cancelable promise but as the narrower Promise type:

function f(): Promise<number> {
  return cancelable(Promise.resolve(666))
}

But it fails because the TS default types includes:

interface Promise<T> {
    readonly [Symbol.toStringTag]: string;
}

Could this be added to this library for compatibility with the base promise type?