sindresorhus / p-cancelable

Create a promise that can be canceled
MIT License
441 stars 23 forks source link

Not compatible with global Bluebird promises #21

Open fluggo opened 4 years ago

fluggo commented 4 years ago

This library, and therefore got, can't be used in a program that uses Bluebird promises globally. This is because PCancelable amends its prototype tree to make it a subclass of Promise but never calls the base Promise constructor, making it a false subclass.

The following code demonstrates this:

import Bluebird from 'bluebird';
global.Promise = Bluebird;

import pcancelable from 'p-cancelable';

async function run() {
  return await new pcancelable((resolve, reject) => {
    resolve(true);
  });
}

run().then(_ => console.log(_));

The code produces no output. It should print true.

sindresorhus commented 4 years ago

It's like this because properly subclassing Promise was very buggy when I first made this package. I hope it has improved since then.

bisubus commented 4 years ago

@fluggo The code you posted may work differently depending on whether you transpile it (native async uses native promises instead of BB, native imports are hoisted). But it seems to me that it should work as expected any way. Parent constructor is called by PCancelable, this._promise = new Promise(...). It won't be play well with Bluebird for other reasons, it's unaware of extra methods if you use them.

@sindresorhus Promise class inheritance is stable now with extends. Similarly to other native classes, it is incompatible with ES5, this would prevent it from working even if they are transpiled. Any way, current implementation is good enough, Promise constructor is called and proto chain is set. setPrototypeOf(PC, Promise) could be added for static methods. This won't improve BB inheritance because the latter wasn't written with inheritance in mind. A proper failsafe approach would be to wrap all static and instance methods with (...args) { return PC.resolve(superMethod(...args) }. I'm not sure if extra resolve call is worth it.

I checked this issue for another reason. PC is currently incompatible with BB cancellation, http://bluebirdjs.com/docs/api/cancellation.html . It doesn't propagate cancelability to promise chain (this could be the subject for PC.resolve()) and thus doesn't implement the strategy for multiple consumers. Do you have plans in this regard?

fluggo commented 4 years ago

@bisubus It's incompatible because Bluebird expects this to be Bluebird, and it isn't. You've called the constructor on a different object, not the PCancelable which says it's an instance of Bluebird.

bisubus commented 4 years ago

@fluggo It may fail in some more complicated cases where BB relies on instanceof checks internally, I'd love to see them as I'm potentially in the same boat. But when promises are only chained with then, it seems pretty much straightforward, the snippet is workable, see https://repl.it/repls/SuddenPrizeOutcomes

fluggo commented 4 years ago

@bisubus Bluebird uses instanceof throughout. This is the main check that tells Bluebird no conversion is necessary, and then it proceeds to call internal methods that are in the PCancelable instance's prototype chain, but which don't work because the object's attributes were never set up by the Bluebird constructor.