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

Make `multiArgs` option apply on rejections too #35

Merged
merged 3 commits into from Apr 18, 2017

Conversation

3 participants
@mightyiam
Contributor

mightyiam commented Jan 20, 2017

No description provided.

@mightyiam

This comment has been minimized.

Show comment
Hide comment
@mightyiam

mightyiam Jan 20, 2017

Contributor

Until we have promises in node's core API things, pify seems like the go-to. Thank you, @sindresorhus .

Are you familiar with this case?

A callback function may be called with err and also provide more useful arguments, as is the case in child_process.execFile.

Contributor

mightyiam commented Jan 20, 2017

Until we have promises in node's core API things, pify seems like the go-to. Thank you, @sindresorhus .

Are you familiar with this case?

A callback function may be called with err and also provide more useful arguments, as is the case in child_process.execFile.

@@ -11,17 +11,23 @@ const processFn = (fn, opts) => function () {
return new P((resolve, reject) => {
args.push(function (err, result) {
- if (err) {
- reject(err);

This comment has been minimized.

@mightyiam

mightyiam Jan 20, 2017

Contributor

This means that when there is an error, multiArgs is not respected, and the rejection reason is the error, and the rest of the callback arguments are lost.

@mightyiam

mightyiam Jan 20, 2017

Contributor

This means that when there is an error, multiArgs is not respected, and the rejection reason is the error, and the rest of the callback arguments are lost.

- resolve(results);
+ if (err) {
+ results.unshift(err);
+ reject(results);

This comment has been minimized.

@mightyiam

mightyiam Jan 20, 2017

Contributor

This means that in the case of an error, multiArgs is respected and the err along with the rest of the callback arguments are provided as the rejection reason.

The spec says (I made the important bit bold):

The reject function that is passed to an executor function accepts a single argument. The executor code may eventually call the reject function to indicate that the associated Promise is rejected and will never be fulfilled. The argument passed to the reject function is used as the rejection value of the promise. Typically it will be an Error object.

I guess that in this particular use case, where we map a callback API to a promise API, this could be the best implementation of the spec, in case multiArgs is passed.

Oh, and since we already are doing multiArgs for resolved promises, why not for rejected ones?

@mightyiam

mightyiam Jan 20, 2017

Contributor

This means that in the case of an error, multiArgs is respected and the err along with the rest of the callback arguments are provided as the rejection reason.

The spec says (I made the important bit bold):

The reject function that is passed to an executor function accepts a single argument. The executor code may eventually call the reject function to indicate that the associated Promise is rejected and will never be fulfilled. The argument passed to the reject function is used as the rejection value of the promise. Typically it will be an Error object.

I guess that in this particular use case, where we map a callback API to a promise API, this could be the best implementation of the spec, in case multiArgs is passed.

Oh, and since we already are doing multiArgs for resolved promises, why not for rejected ones?

@schnittstabil

This comment has been minimized.

Show comment
Hide comment
@schnittstabil

schnittstabil Jan 20, 2017

Collaborator

@mightyiam While that sounds reasonable, I do not see any tests reflecting your change and the use-cases you had in mind.

Collaborator

schnittstabil commented Jan 20, 2017

@mightyiam While that sounds reasonable, I do not see any tests reflecting your change and the use-cases you had in mind.

@mightyiam

This comment has been minimized.

Show comment
Hide comment
@mightyiam

mightyiam Jan 20, 2017

Contributor

Does this make sense?

First glance at the tests did not make any sense to me, @schnittstabil. I'll give it another read. Any pointers?

Contributor

mightyiam commented Jan 20, 2017

Does this make sense?

First glance at the tests did not make any sense to me, @schnittstabil. I'll give it another read. Any pointers?

@mightyiam

This comment has been minimized.

Show comment
Hide comment
@mightyiam

mightyiam Jan 20, 2017

Contributor

I think I get it now. Test should be coming up soon.

Contributor

mightyiam commented Jan 20, 2017

I think I get it now. Test should be coming up soon.

index.js
} else {
+ if (err) {
+ reject(err);
+ }
resolve(result);

This comment has been minimized.

@schnittstabil

schnittstabil Jan 20, 2017

Collaborator

Because of semantic of Promises this is maybe not a bug, but I believe it would be more clear to readers, if we have:

+				if (err) {
+					reject(err);
+				} else {
  					resolve(result);
+				}
@schnittstabil

schnittstabil Jan 20, 2017

Collaborator

Because of semantic of Promises this is maybe not a bug, but I believe it would be more clear to readers, if we have:

+				if (err) {
+					reject(err);
+				} else {
  					resolve(result);
+				}

This comment has been minimized.

@mightyiam

mightyiam Jan 20, 2017

Contributor

You're absolutely right.

@mightyiam

mightyiam Jan 20, 2017

Contributor

You're absolutely right.

@@ -42,6 +47,10 @@ test('multiArgs option', async t => {
t.deepEqual(await fn(fixture3, {multiArgs: true})(), ['unicorn', 'rainbow']);
});
+test('multiArgs option — rejection', async t => {
+ t.deepEqual(await fn(fixture1, {multiArgs: true})().catch(err => err), ['error', 'unicorn', 'rainbow']);
+});

This comment has been minimized.

@mightyiam

mightyiam Jan 20, 2017

Contributor

Got a test in place for this use case.

@mightyiam

mightyiam Jan 20, 2017

Contributor

Got a test in place for this use case.

@@ -30,6 +31,10 @@ test('main', async t => {
t.is(await fn(fixture)(), 'unicorn');
});
+test('error', async t => {
+ t.is(await fn(fixture1)().catch(err => err), 'error');
+});

This comment has been minimized.

@mightyiam

mightyiam Jan 20, 2017

Contributor

And threw in a bonus test for the basic error (without multiArgs).

@mightyiam

mightyiam Jan 20, 2017

Contributor

And threw in a bonus test for the basic error (without multiArgs).

@@ -4,6 +4,7 @@ import pinkiePromise from 'pinkie-promise';
import fn from './';
const fixture = cb => setImmediate(() => cb(null, 'unicorn'));
+const fixture1 = cb => setImmediate(() => cb('error', 'unicorn', 'rainbow'));

This comment has been minimized.

@mightyiam

mightyiam Jan 20, 2017

Contributor

This fits right in 😉

@mightyiam

mightyiam Jan 20, 2017

Contributor

This fits right in 😉

@mightyiam mightyiam referenced this pull request in mightyiam/curated-linter Jan 21, 2017

Open

Use pify proper #20

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Apr 18, 2017

Owner

I intentionally didn't do multiArgs for rejections, but your arguments makes sense.

Owner

sindresorhus commented Apr 18, 2017

I intentionally didn't do multiArgs for rejections, but your arguments makes sense.

@sindresorhus sindresorhus changed the title from multiargs on error to Make `multiArgs` option apply on rejections too Apr 18, 2017

@sindresorhus sindresorhus merged commit 30116d9 into sindresorhus:master Apr 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mightyiam mightyiam deleted the mightyiam:multiargs-error branch Apr 18, 2017

@mightyiam

This comment has been minimized.

Show comment
Hide comment
@mightyiam

mightyiam Apr 18, 2017

Contributor

Thanks a bunch.

Contributor

mightyiam commented Apr 18, 2017

Thanks a bunch.

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