Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not compatible with global Bluebird promises #21

Open
fluggo opened this issue Nov 11, 2019 · 5 comments
Open

Not compatible with global Bluebird promises #21

fluggo opened this issue Nov 11, 2019 · 5 comments

Comments

@fluggo
Copy link

fluggo commented Nov 11, 2019

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
Copy link
Owner

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
Copy link

bisubus commented Nov 23, 2019

@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
Copy link
Author

fluggo commented Nov 23, 2019

@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
Copy link

bisubus commented Nov 24, 2019

@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
Copy link
Author

fluggo commented Nov 28, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants