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

Rejected promises log true #133

Open
devinus opened this issue Mar 12, 2016 · 6 comments
Open

Rejected promises log true #133

devinus opened this issue Mar 12, 2016 · 6 comments

Comments

@devinus
Copy link

devinus commented Mar 12, 2016

vorpal
.command('fail me <arg>')
.description('Must return an arg.')
.action(function (args) {
  return new Promise(function (resolve, reject) {
    if (args.arg === 'not') {
      resolve('we are happy');
    } else {
      reject('we are not happy.');
    }
  });
});
$ fail me

  Missing required argument. Showing Help:

  Usage: fail me [options] <arg>

  Must return an arg.

  Options:

    --help  output usage information

$ fail me foo
true
@devinus
Copy link
Author

devinus commented Mar 12, 2016

This happens here:

var err = response.error;

response is { error: true, data: 'we are not happy.', args: undefined }.

Perhaps error.data is what should be logged?

@devinus
Copy link
Author

devinus commented Mar 12, 2016

And only when error.data is truthy? Unsure. Seems like it should only this.log if it's an actual instanceof Error.

@scotthovestadt
Copy link
Contributor

@devinus Can you create a failing test? If there's a bug with a failing test I will fix it.

@zakhenry
Copy link
Contributor

zakhenry commented Jun 18, 2016

See the catch block where the promise rejection is handled. https://github.com/dthree/vorpal/blob/62f77e1477ec11245c320b5d1d56b521c36ea6d8/lib/session.js#L482-488

The second param of onCompletion (data) is set to true, which is what ends up being output. It's worth noting that a non-promise callback that throws new Error will cause an exit 1.
This can be replicated by replacing the onCompletion with timeout to throw error out of the promise chain.

if (res && _.isFunction(res.then)) {
    res.then(function (data) {
      onCompletion(wrapper, undefined, data);
    }).catch(function (err) {
      // onCompletion(wrapper, true, err);
      setTimeout(() => {
        throw err
      });
    });
  }

I'm still not sure on what the intended behaviour is though, but exiting with 0 and logging 'true' cant be it.

@weinma
Copy link

weinma commented Jun 27, 2016

Hi! My personal workaround for now:

I changed the onCompletet function in session.js:444 to:

function onCompletion(wrapper, err, data, argus) {
    response = {
      error: err ? data : null,
      data: data,
      args: argus
    };
    self.completeCommand();
  }

Note: I needed a workaround fast, so i didn't get completely into it. I just needed it to work for my cause.

@ahz
Copy link

ahz commented Dec 29, 2016

According to comments in #35 the workaround can also be to reject promises with new Error().

vorpal
.command('fail me <arg>')
.description('Must return an arg.')
.action(function (args) {
  return new Promise(function (resolve, reject) {
    if (args.arg === 'not') {
      resolve('we are happy');
    } else {
      reject(new Error('we are not happy.')); // like this
    }
  });
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants