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

Inconsistent propagation of promise.bind() #738

Closed
jiangfengming opened this Issue Aug 24, 2015 · 9 comments

Comments

Projects
None yet
5 participants
@jiangfengming

jiangfengming commented Aug 24, 2015

Promise.resolve().then(function() {
  return new Promise(function(resolve) {
    resolve();
  }).bind({ a: 1 });
}).then(function() {
  console.log(this);
});

Outputs window.

However appending a then() makes it work:

Promise.resolve().then(function() {
  return new Promise(function(resolve) {
    resolve();
  }).bind({ a: 1 }).then(function() {});
}).then(function() {
  console.log(this);
});
@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Aug 24, 2015

Contributor

Interesting, thanks. However I'd argue that the first is correct behaviour and the propagation of the bound value is the bug. bind should only influence linear chains. Consider

Promise.bind(a).then(function() {
    return Promise.bind(b);
}).then(function() {
    console.log(this); // a or b?
})

Especially when some of your handlers are returning promises that you didn't create yourself, you'll want it to be a.

Contributor

bergus commented Aug 24, 2015

Interesting, thanks. However I'd argue that the first is correct behaviour and the propagation of the bound value is the bug. bind should only influence linear chains. Consider

Promise.bind(a).then(function() {
    return Promise.bind(b);
}).then(function() {
    console.log(this); // a or b?
})

Especially when some of your handlers are returning promises that you didn't create yourself, you'll want it to be a.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 24, 2015

Collaborator

@bergus it should be a. That makes sense for the arguments you've yourself said.

Collaborator

benjamingr commented Aug 24, 2015

@bergus it should be a. That makes sense for the arguments you've yourself said.

@benjamingr benjamingr closed this Aug 24, 2015

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Aug 24, 2015

Contributor

@benjamingr Yes, but the case that "works" for the OP is a bug, isn't it?

var dummy = { a: 1 };
Promise.resolve().then(function() {
  return Promise.bind(dummy).then(function() {});
}).then(function() {
  console.assert(this !== dummy, "callback should not be bound");
});

Especially that it works when .then(function() {}); is commented out makes this suspicious.

Contributor

bergus commented Aug 24, 2015

@benjamingr Yes, but the case that "works" for the OP is a bug, isn't it?

var dummy = { a: 1 };
Promise.resolve().then(function() {
  return Promise.bind(dummy).then(function() {});
}).then(function() {
  console.assert(this !== dummy, "callback should not be bound");
});

Especially that it works when .then(function() {}); is commented out makes this suspicious.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Aug 24, 2015

Collaborator

Reopening because either the first behaviour is a bug, or the second one is 😄

Collaborator

spion commented Aug 24, 2015

Reopening because either the first behaviour is a bug, or the second one is 😄

@spion spion reopened this Aug 24, 2015

@spion spion changed the title from Returning promise.bind() in then() looses "this" to Inconsistent propagation of promise.bind() Aug 24, 2015

@jiangfengming

This comment has been minimized.

Show comment
Hide comment
@jiangfengming

jiangfengming Aug 25, 2015

@bergus Yes, I agree. But propagating this to the upper promise chain is useful. Consider the following:

function wrappedApi(args) {
  var _resolve;
  var _reject;
  var promise = new Promise(function(resolve, reject) {
    _resolve = resolve;
    _reject = reject;
  });

  return prerequisite().then(function() {
    return ajax(args).then(function(val) {
      _resolve(this);
      return val;
    }, function(e) {
      _resolve(this);
      throw e;
    });
  }, function(e) {
    _reject(e);
    throw e;
  }).bind(promise);
}

wrappedApi({ foo: 1 }).then(function(result) {
  console.log(this.responseText);
});

If there is a method to propagate this to the upper chain, then life would be easier.

function wrappedApi(args) {
  return prerequisite().then(function() {
    return ajax(args).propagateContext();
  });
}

jiangfengming commented Aug 25, 2015

@bergus Yes, I agree. But propagating this to the upper promise chain is useful. Consider the following:

function wrappedApi(args) {
  var _resolve;
  var _reject;
  var promise = new Promise(function(resolve, reject) {
    _resolve = resolve;
    _reject = reject;
  });

  return prerequisite().then(function() {
    return ajax(args).then(function(val) {
      _resolve(this);
      return val;
    }, function(e) {
      _resolve(this);
      throw e;
    });
  }, function(e) {
    _reject(e);
    throw e;
  }).bind(promise);
}

wrappedApi({ foo: 1 }).then(function(result) {
  console.log(this.responseText);
});

If there is a method to propagate this to the upper chain, then life would be easier.

function wrappedApi(args) {
  return prerequisite().then(function() {
    return ajax(args).propagateContext();
  });
}
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Aug 25, 2015

Owner

The intention has never been so that unknown code could change this for you. In your example it would be less confusing if you did wrappedApi().then(result => console.log(result.responseText))

Owner

petkaantonov commented Aug 25, 2015

The intention has never been so that unknown code could change this for you. In your example it would be less confusing if you did wrappedApi().then(result => console.log(result.responseText))

@jiangfengming

This comment has been minimized.

Show comment
Hide comment
@jiangfengming

jiangfengming Aug 25, 2015

@petkaantonov The result is not the XHR object. How to resolve the promise with JSON.parse(req.responseText) and bind this to the XHR object ?

jiangfengming commented Aug 25, 2015

@petkaantonov The result is not the XHR object. How to resolve the promise with JSON.parse(req.responseText) and bind this to the XHR object ?

@bergus

This comment has been minimized.

Show comment
Hide comment
@bergus

bergus Aug 25, 2015

Contributor

I don't think you should do that at all. If prerequisite fails, there is no XHR object to bind to… which does not only lead to your confusing wrappedApi code, but also makes its usage complicated.
I'd probably either return [json, XHR] tuples and use spread, or let the user explicitly write out prerequisite and the nested then calls (which can handle different errors cleanly).

Contributor

bergus commented Aug 25, 2015

I don't think you should do that at all. If prerequisite fails, there is no XHR object to bind to… which does not only lead to your confusing wrappedApi code, but also makes its usage complicated.
I'd probably either return [json, XHR] tuples and use spread, or let the user explicitly write out prerequisite and the nested then calls (which can handle different errors cleanly).

@jiangfengming

This comment has been minimized.

Show comment
Hide comment
@jiangfengming

jiangfengming Aug 26, 2015

@bergus If prerequisite fails, you can handle it inside wrappedApi or in global unhandledrejection. In wrappedApi().catch(), you should check the error code, not handle prerequisite errors and re-throw it.

Returning [result, thisObj] is weird, and thisObj will lost in the following chain... We should hide the complexity in the low level. If every remote API needs to check prerequisite, like oAuth2, you should check access token and expires and refresh the token if necessary, your every API call will become rpc.auth.check().then(function() { rpc.something.get().then(...).catch(...) }).catch(...), and your users will shit on you.

jiangfengming commented Aug 26, 2015

@bergus If prerequisite fails, you can handle it inside wrappedApi or in global unhandledrejection. In wrappedApi().catch(), you should check the error code, not handle prerequisite errors and re-throw it.

Returning [result, thisObj] is weird, and thisObj will lost in the following chain... We should hide the complexity in the low level. If every remote API needs to check prerequisite, like oAuth2, you should check access token and expires and refresh the token if necessary, your every API call will become rpc.auth.check().then(function() { rpc.something.get().then(...).catch(...) }).catch(...), and your users will shit on you.

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