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

`promisify()` behavior with multiple parameters can break compatibility #307

Closed
julien-f opened this Issue Aug 28, 2014 · 25 comments

Comments

Projects
None yet
7 participants
@julien-f

julien-f commented Aug 28, 2014

Current behavior of promisify is to resolve to an array when there is more than one result to the callback.

Unfortunately, if a function is changed to return two or more results instead of one, it breaks the compatibility of the promisified version.

var assert = require('assert');
var promisify = require('bluebird').promisify;

// Original function returning one result.
function version1(cb) {
  process.nextTick(function () {
    cb(null, 'foo');
  });
}

// Updated function returning two results.
function version2(cb) {
  process.nextTick(function () {
    cb(null, 'foo', 'bar');
  });
}

// The compatibility is maintained when using callbacks.
function validateWithCallback(fn) {
  fn(function (err, foo) {
    assert.equal(foo, 'foo');
  });
}
validateWithCallback(version1);
validateWithCallback(version2);

// The compatibility is broken when using the promisified version.
function validateWithPromise(fn) {
  promisify(fn)().then(function (foo) {
    assert.equal(foo, 'foo');
  });
}
validateWithPromise(version1);
validateWithPromise(version2);

I think the correct approach would be to have an option to force to either return only the first result or to always return an array.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 29, 2014

Collaborator

I'm not sure what the problem is. If you want this behavior then use .spread instead of .then when working with arrays - it simulates destructuring.

Collaborator

benjamingr commented Aug 29, 2014

I'm not sure what the problem is. If you want this behavior then use .spread instead of .then when working with arrays - it simulates destructuring.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Sep 1, 2014

Owner

If we changed .spread to behave like .then when the input is not an array, could it break reasonable code?

So if a multi-argument callback function is changed to single-argument, all code using spread would keep working.

The other way around I.E. changing a callback function from a conforming node callback convention (1-arg) to a non standard custom convention (multi-args) is completely unreasonable and will not be supported.

Owner

petkaantonov commented Sep 1, 2014

If we changed .spread to behave like .then when the input is not an array, could it break reasonable code?

So if a multi-argument callback function is changed to single-argument, all code using spread would keep working.

The other way around I.E. changing a callback function from a conforming node callback convention (1-arg) to a non standard custom convention (multi-args) is completely unreasonable and will not be supported.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 1, 2014

Collaborator

What would ES6 destructuring do with a single argument? If I had to guess - I'd guess run-time error when trying to access a non-array like an array in the best case, undefined in the slightly worse case and the worst option is getting "a" when trying to destructure var [x] = "abcde";.

I don't think the use case for this (the underlying node function changing its signature) is worth the ambiguity this can cause.

Collaborator

benjamingr commented Sep 1, 2014

What would ES6 destructuring do with a single argument? If I had to guess - I'd guess run-time error when trying to access a non-array like an array in the best case, undefined in the slightly worse case and the worst option is getting "a" when trying to destructure var [x] = "abcde";.

I don't think the use case for this (the underlying node function changing its signature) is worth the ambiguity this can cause.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 1, 2014

Collaborator

Ok - so I checked and that's what it does:

    var [x] = 3; // x is `undefined`
    var [x] = "Hello"; // x is "H"
    var [x] = []; // x is undefined
Collaborator

benjamingr commented Sep 1, 2014

Ok - so I checked and that's what it does:

    var [x] = 3; // x is `undefined`
    var [x] = "Hello"; // x is "H"
    var [x] = []; // x is undefined
@julien-f

This comment has been minimized.

Show comment
Hide comment
@julien-f

julien-f Sep 3, 2014

I know that this is a small issue but I do think that it may leads to problems.

1. Compatibility break

I maintain that, like in the example of my first comment, it is perfectly logical for a developer to expect that returning 2 values instead of one in callback will not break compatibility.

The user of such a code should not see its own code broken just because he used a promisified version of this async function.

2. Unpredictability for callback with variable number of arguments

Even if not a good practice, it is perfectly possible to create an async function which resolve to a variable number of results.

With the current behavior, promisify simply cannot be used either chained by then() or spread():

  • then(): impossible to tell the difference between multiple results or one array;
  • spread(): it's even worst:
    • works with multiple results;
    • incorrectly spread values of a single array;
    • throws a TypeError if the single value is not an array.

I agree that I may nitpicking but having a predictable behavior for this method would be nice :)

julien-f commented Sep 3, 2014

I know that this is a small issue but I do think that it may leads to problems.

1. Compatibility break

I maintain that, like in the example of my first comment, it is perfectly logical for a developer to expect that returning 2 values instead of one in callback will not break compatibility.

The user of such a code should not see its own code broken just because he used a promisified version of this async function.

2. Unpredictability for callback with variable number of arguments

Even if not a good practice, it is perfectly possible to create an async function which resolve to a variable number of results.

With the current behavior, promisify simply cannot be used either chained by then() or spread():

  • then(): impossible to tell the difference between multiple results or one array;
  • spread(): it's even worst:
    • works with multiple results;
    • incorrectly spread values of a single array;
    • throws a TypeError if the single value is not an array.

I agree that I may nitpicking but having a predictable behavior for this method would be nice :)

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 3, 2014

Collaborator

@julien-f I should have clarified my former message:

  • The behavior we have right now is consistent. Although it might not be the most intuitive it is predictable and acts exactly like function returns in JS (You can't return multiple values so you return an array of them)
  • The purpose of .spread is to act as a shim for ES6 destructuring. The behavior of spread should stay consistent with ES6 destructuring. ES6 destructuring would not ignore the value not being an array so neither should spread - the alternative to the TypeError would be undefined and not unwrapping the value - I don't think the use case for unwrapping to undefined is strong enough.

If you have a method that returns different return types every time - there is no way around using then and then branching to cases.

If you have a method whose signature changes - the caller site has to be aware. There is no way around it.

Collaborator

benjamingr commented Sep 3, 2014

@julien-f I should have clarified my former message:

  • The behavior we have right now is consistent. Although it might not be the most intuitive it is predictable and acts exactly like function returns in JS (You can't return multiple values so you return an array of them)
  • The purpose of .spread is to act as a shim for ES6 destructuring. The behavior of spread should stay consistent with ES6 destructuring. ES6 destructuring would not ignore the value not being an array so neither should spread - the alternative to the TypeError would be undefined and not unwrapping the value - I don't think the use case for unwrapping to undefined is strong enough.

If you have a method that returns different return types every time - there is no way around using then and then branching to cases.

If you have a method whose signature changes - the caller site has to be aware. There is no way around it.

@julien-f

This comment has been minimized.

Show comment
Hide comment
@julien-f

julien-f Sep 3, 2014

@benjamingr I understand what you mean but I disagree: a callback with multiple values is not the same as returning an array, there is a transformation.

I perfectly see the dilemma for promisify(): how to be able to forward all results of a callback into one returned value.

What I don't like in the current implementation is the fact there are two different behaviors (forwards the only value or wrapped multiple values in an array) and there is no way to tell from the result which one has been used.

Maybe promisify() should explicitly ignore multiple values or maybe there could be an option to force one behavior or the other.

(I am sure I am driving you crazy ^^, feel free to close the issue if you consider this a non issue.)

julien-f commented Sep 3, 2014

@benjamingr I understand what you mean but I disagree: a callback with multiple values is not the same as returning an array, there is a transformation.

I perfectly see the dilemma for promisify(): how to be able to forward all results of a callback into one returned value.

What I don't like in the current implementation is the fact there are two different behaviors (forwards the only value or wrapped multiple values in an array) and there is no way to tell from the result which one has been used.

Maybe promisify() should explicitly ignore multiple values or maybe there could be an option to force one behavior or the other.

(I am sure I am driving you crazy ^^, feel free to close the issue if you consider this a non issue.)

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 3, 2014

Collaborator

Let me reiterate what @petkaantonov said earlier - A callback with multiple values does not abide to the nodejs callback convention to begin with.

You're also welcome to pass a custom promisifier to Bluebird or promisify things manually yourself if you don't like how Bluebird handles promisification.

Collaborator

benjamingr commented Sep 3, 2014

Let me reiterate what @petkaantonov said earlier - A callback with multiple values does not abide to the nodejs callback convention to begin with.

You're also welcome to pass a custom promisifier to Bluebird or promisify things manually yourself if you don't like how Bluebird handles promisification.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 3, 2014

In Q v2 and in RSVP we handle this by ignoring any arguments after the first by default, but giving you the option to opt-in via a second parameter to denodeify.

domenic commented Sep 3, 2014

In Q v2 and in RSVP we handle this by ignoring any arguments after the first by default, but giving you the option to opt-in via a second parameter to denodeify.

@julien-f

This comment has been minimized.

Show comment
Hide comment
@julien-f

julien-f Sep 3, 2014

@domenic It is IMHO the correct way to do it.

julien-f commented Sep 3, 2014

@domenic It is IMHO the correct way to do it.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Sep 3, 2014

Owner

I think option is fine but defaulting to information loss and inconvenience (the common case arg of request is the second one) is absurd.

Owner

petkaantonov commented Sep 3, 2014

I think option is fine but defaulting to information loss and inconvenience (the common case arg of request is the second one) is absurd.

@phpnode

This comment has been minimized.

Show comment
Hide comment
@phpnode

phpnode Sep 3, 2014

it should be left as is. Passing in a flag is absurd, it's up to the consumer of the promise to decide what to do with the result it receives. There's no way of avoiding knowing what types you're dealing with, and if you have a badly behaved function like this you can always append your own .then() handler which gives you what you were looking for.

phpnode commented Sep 3, 2014

it should be left as is. Passing in a flag is absurd, it's up to the consumer of the promise to decide what to do with the result it receives. There's no way of avoiding knowing what types you're dealing with, and if you have a badly behaved function like this you can always append your own .then() handler which gives you what you were looking for.

@julien-f

This comment has been minimized.

Show comment
Hide comment
@julien-f

julien-f Sep 3, 2014

@phpnode Current behavior would be kept and the optional flag should be used only when you want to wrap the values in an array to avoid ambiguities.

I do not see the problem of adding this flag which may be helpful to some.

julien-f commented Sep 3, 2014

@phpnode Current behavior would be kept and the optional flag should be used only when you want to wrap the values in an array to avoid ambiguities.

I do not see the problem of adding this flag which may be helpful to some.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Sep 3, 2014

Owner

@julien-f The current behavior is to wrap multiple arguments in an array. An option would simply just drop the extra arguments. However, it feels like this all is hypothetical so I might delay adding it until someone has a real issue with it.

Owner

petkaantonov commented Sep 3, 2014

@julien-f The current behavior is to wrap multiple arguments in an array. An option would simply just drop the extra arguments. However, it feels like this all is hypothetical so I might delay adding it until someone has a real issue with it.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Sep 3, 2014

It was a real issue for Q. Bower used Q v1 (which has the same behavior as Bluebird currently does) and pinned a dependency to something like 1.2.x. That dependency added a third callback parameter between 1.2.1 and 1.2.2, because it is not normally a backward-incompatible change. But all of a sudden all new bower installations were broken since the denodeified version of the function was now calling with an array instead of a string or something. bower/bower#1403

domenic commented Sep 3, 2014

It was a real issue for Q. Bower used Q v1 (which has the same behavior as Bluebird currently does) and pinned a dependency to something like 1.2.x. That dependency added a third callback parameter between 1.2.1 and 1.2.2, because it is not normally a backward-incompatible change. But all of a sudden all new bower installations were broken since the denodeified version of the function was now calling with an array instead of a string or something. bower/bower#1403

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Sep 3, 2014

Owner

Technically almost anything is backward-incompatible. For example, if someone has code that is for some reason passing 2 arguments to function that only expects one, it will very likely break when the function is extended to support a second argument. And so on. So it's only about what is reasonable and common, otherwise semver is meaningless as every change has to be a major.

However, tmp was in the wrong here. I can understand fixing your callbacks to adhere to the callback convention but I cannot understand making a change that violates the convention.

Owner

petkaantonov commented Sep 3, 2014

Technically almost anything is backward-incompatible. For example, if someone has code that is for some reason passing 2 arguments to function that only expects one, it will very likely break when the function is extended to support a second argument. And so on. So it's only about what is reasonable and common, otherwise semver is meaningless as every change has to be a major.

However, tmp was in the wrong here. I can understand fixing your callbacks to adhere to the callback convention but I cannot understand making a change that violates the convention.

@julien-f

This comment has been minimized.

Show comment
Hide comment
@julien-f

julien-f Sep 3, 2014

@petkaantonov I meant a flag to always wrap in an array, even if only one item, but what you proposing also works, we simply need a way to force the behavior used :)

julien-f commented Sep 3, 2014

@petkaantonov I meant a flag to always wrap in an array, even if only one item, but what you proposing also works, we simply need a way to force the behavior used :)

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Sep 3, 2014

Collaborator

@petkaantonov the point @domenic is trying to make if I understand correctly is that the library authors did not expect to break anything - as far as they were concerned the change they made added functionality but did not break any. It ending up breaking nodeify was a surprise to their eyes.

While I don't think changing the behavior is a good idea at this point (and I like the current behavior) I see the point here.

Collaborator

benjamingr commented Sep 3, 2014

@petkaantonov the point @domenic is trying to make if I understand correctly is that the library authors did not expect to break anything - as far as they were concerned the change they made added functionality but did not break any. It ending up breaking nodeify was a surprise to their eyes.

While I don't think changing the behavior is a good idea at this point (and I like the current behavior) I see the point here.

@nsabovic

This comment has been minimized.

Show comment
Hide comment
@nsabovic

nsabovic Mar 13, 2015

I've been hit by this when using the mongodb driver.

Depending on the operation and result, it sometimes calls the callback with two parameters and sometimes with three. So I will sometimes get back a promise that resolves to one value (which I need), and sometimes to array of two values, of which the second one I don't need. Now the problem is that the value that I need can sometimes be an array, too. So when I get an array, I have no way of knowing if it's the result that I want (that's an array), or if the promisify function jumbled the other parameter in there too.

The root of the problem is that the shape of the value is decided at the invocation time instead of at the promisification time, and there's no way to make it fixed (either give me an array always, or give me just the second parameter always).

Sorry for resurrecting an ancient issue.

nsabovic commented Mar 13, 2015

I've been hit by this when using the mongodb driver.

Depending on the operation and result, it sometimes calls the callback with two parameters and sometimes with three. So I will sometimes get back a promise that resolves to one value (which I need), and sometimes to array of two values, of which the second one I don't need. Now the problem is that the value that I need can sometimes be an array, too. So when I get an array, I have no way of knowing if it's the result that I want (that's an array), or if the promisify function jumbled the other parameter in there too.

The root of the problem is that the shape of the value is decided at the invocation time instead of at the promisification time, and there's no way to make it fixed (either give me an array always, or give me just the second parameter always).

Sorry for resurrecting an ancient issue.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 13, 2015

Owner

@nsabovic Yes this is fixed in 3.0, it cannot be fixed in 2.x because it's a blatantly backward incompatible change

Owner

petkaantonov commented Mar 13, 2015

@nsabovic Yes this is fixed in 3.0, it cannot be fixed in 2.x because it's a blatantly backward incompatible change

@nsabovic

This comment has been minimized.

Show comment
Hide comment
@nsabovic

nsabovic Mar 24, 2015

Awesome! Where is 3.0? npm has 2.9 something, 3.0 branch in github also says 2.9 in package.json...

nsabovic commented Mar 24, 2015

Awesome! Where is 3.0? npm has 2.9 something, 3.0 branch in github also says 2.9 in package.json...

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Mar 24, 2015

Owner

3.0 is not released yet, it's under development

Owner

petkaantonov commented Mar 24, 2015

3.0 is not released yet, it's under development

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Oct 27, 2015

Owner

Fixed in 3.0 release

Owner

petkaantonov commented Oct 27, 2015

Fixed in 3.0 release

@gajus

This comment has been minimized.

Show comment
Hide comment
@gajus

gajus Nov 10, 2015

@petehunt can you give some insights into how mysql is supposed to be promisified now?

gajus commented Nov 10, 2015

@petehunt can you give some insights into how mysql is supposed to be promisified now?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Nov 11, 2015

Owner

@gajus same as before just dont use spread with query

Owner

petkaantonov commented Nov 11, 2015

@gajus same as before just dont use spread with query

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