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

Async-listener breaks method dispatch for promise subclasses #112

Closed
matthewloring opened this issue Jun 7, 2017 · 7 comments
Closed

Async-listener breaks method dispatch for promise subclasses #112

matthewloring opened this issue Jun 7, 2017 · 7 comments

Comments

@matthewloring
Copy link
Contributor

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

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 added a commit to googleapis/cloud-trace-nodejs that referenced this issue Jun 8, 2017
New versions of got do not play well with async listener. The bug
tracking this issue is here:
othiym23/async-listener#112.

Got is only a dev dependency used by the system tests so relying on an
older version should be fine.

PR-URL: #504
@matthewloring
Copy link
Contributor Author

@watson @Qard @othiym23 Any thoughts on this?

@watson
Copy link
Collaborator

watson commented Jun 15, 2017

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

@matthewloring
Copy link
Contributor Author

Can we re-opening after revert of the original fix due to #121. I don't have permission.

@hayes
Copy link
Collaborator

hayes commented Jun 23, 2017

@watson opened a new issue #123

@watson
Copy link
Collaborator

watson commented Jun 23, 2017

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

@hayes
Copy link
Collaborator

hayes commented Jun 23, 2017

I think the new issue is good

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

No branches or pull requests

3 participants