othiym23 / async-listener

polyfill version of the 0.11 version of the asyncListener API
https://www.npmjs.org/package/async-listener
BSD 2-Clause "Simplified" License
174 stars 52 forks source link

Async-listener breaks method dispatch for promise subclasses #112

Closed matthewloring closed 7 years ago

matthewloring commented 7 years ago

Test case:

require('async-listener');
class P extends Promise {
  then() {
    console.log('In Subclass');
    return super.then.apply(this, arguments);
  }
}

P.resolve(5).then(v => {
  console.log(v);
});

Expected output (and observed when require('async-listener'); is removed):

> In Subclass
> 5

Observed output:

> 5

This issue breaks the got module (by breaking p-cancelable) and can be reproduced with node version 6.x and 8.x.

matthewloring commented 7 years ago

I believe this snippet illustrates the underlying problem:

function WrappedPromise(executor) {
  var p = new Promise(executor);
  p.__proto__ = WrappedPromise.prototype;
  return p;
}

WrappedPromise.__proto__ = Promise;
WrappedPromise.prototype.__proto__ = Promise.prototype;

WrappedPromise.prototype.then = function then(onFulfilled, onRejected) {
  console.log('WrappedPromiseThen');
  return Promise.prototype.then.call(this, onFulfilled, onRejected);
};

class P extends WrappedPromise {
  then(onF, onR) {
    console.log('PThen');
    return Promise.prototype.then.call(this, onF, onR);
  }
}

(new P(res => { res(42); })).then(v => { console.log(v); });

Async-listener replaces the global promise with its own function-based class. Other modules extend the global Promise using ES6 class syntax. However, extending the function-based wrapper class does not result in the correct dispatch behavior. I think the best approach may be to rewrite the wrappedPromise as an ES6 class. Does anyone have thoughts on that change or any other possible approaches?

matthewloring commented 7 years ago

@watson @Qard @othiym23 Any thoughts on this?

watson commented 7 years ago

@matthewloring Thanks for the ping 😃 I've commented on the PR

matthewloring commented 7 years ago

Can we re-opening after revert of the original fix due to https://github.com/othiym23/async-listener/issues/121. I don't have permission.

hayes commented 7 years ago

@watson opened a new issue #123

watson commented 7 years ago

I could also just reopen this if you guys think that is better?

hayes commented 7 years ago

I think the new issue is good