dthree / vorpal

Node's framework for interactive CLIs
http://vorpal.js.org
MIT License
5.64k stars 280 forks source link

strange "Vorpal Prompt error" #71

Closed fiatjaf closed 8 years ago

fiatjaf commented 8 years ago

I have this pretty small vorpal app: https://github.com/fiatjaf/fieldbook-cli/tree/b3083a70b3ce12f137c7cb22cbe145eb9b9fce6c. The first command, auth, triggers the creation of new commands (through vorpal.emit() and vorpal.on()), and it goes correctly, but then this bizarre error appears:

fiatjaf@spooner ~/c/fieldbook-cli> 
./bin/executable.js auth --book 564a28s4dsc43200ce3215 --key key-1 --password 7734277-9dALvv70RL2342154v70ZQu
fieldbook~$ 
using book 564a28s4dsc43200ce3215.
Vorpal Prompt error: [Error: using book 564a28s4dsc43200ce3215.]

Which is strange, because my code is not calling .prompt twice.

I put a dummy error (this EXIT() function) on vorpal code to get the complete stack trace for the exact moment the error is triggered, and got the following:

fieldbook~$ 
using book 564a28s4dsc43200ce3215.
Vorpal Prompt error: [Error: using book 564a28s4dsc43200ce3215.]

/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/ui.js:191
      EXIT()
      ^

ReferenceError: EXIT is not defined
    at Object.ui.prompt (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/ui.js:191:7)
    at EventEmitter.vorpal.prompt (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/vorpal.js:488:17)
    at EventEmitter.session.prompt (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/session.js:143:22)
    at CommandInstance.prompt (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/command-instance.js:76:25)
    at CommandInstance.<anonymous> (/home/fiatjaf/comp/fieldbook-cli/index.js:114:14)
    at EventEmitter.session.execCommandSet (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/session.js:479:24)
    at EventEmitter.vorpal._exec (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/vorpal.js:826:18)
    at EventEmitter.vorpal._execQueueItem (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/vorpal.js:639:10)
    at EventEmitter.vorpal._queueHandler (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/vorpal.js:623:10)
    at EventEmitter.vorpal.exec (/home/fiatjaf/comp/fieldbook-cli/node_modules/vorpal/lib/vorpal.js:598:10)

I don't understand it.

dthree commented 8 years ago

That's not an error with the Vorpal prompt itself. This prompt got triggered in a rather broad try / catch statement:

    try {
      prompt = inquirer.prompt(options, function (result) {
        self._midPrompt = false;
        if (self._cancel === true) {
          self._cancel = false;
        } else {
          cb(result);
        }
      });

      // Temporary hack. We need to pull the active
      // prompt from inquirer as soon as possible,
      // however we can't just assign it sync, as
      // the prompt isn't ready yet.
      // I am trying to get inquirer extended to
      // fire an event instead.
      setTimeout(function () {
        // self._activePrompt = prompt._activePrompt;
      }, 100);
    } catch (e) {
      console.log('Vorpal Prompt error:', e);
    }

It looks like your program had an internal error while mid prompt, and so Vorpal just caught the error and decided to be the one to spit the results.


BTW, would you be able to throw this on Stack Overflow? Trying to keep support questions on that site for maximum benefit to the community. I get immediate email triggers on the vorpal.js tag, so I'll definitely see it. Thanks.

dthree commented 8 years ago

Oh I think I found the problem BTW. You're using vorpal.exit(), and this isn't a supported command per the wiki. It's an internal command used by Vantage.js.

Just use process.exit() instead and that should fix your problem.

fiatjaf commented 8 years ago

I'm sorry, I know it is better to post questions on StackOverflow, but I sincerely thought (and maybe still think) this is more a bug then a question.

Thank you for searching the bug for me, but the problem wasn't on process.exit(). It was something on https://github.com/fiatjaf/fieldbook-cli/blob/b3083a70b3ce12f137c7cb22cbe145eb9b9fce6c/index.js#L124. When I replaced

cb(`using book ${args.options.book}.`)

with

this.log(`using book ${args.options.book}.`)
cb()

The error stopped showing. Again, I don't understand.

Also, perhaps the try-catch block should be changed, since it is catching things it should not? Perhaps just removing it, or rethrowing the error, would be a better idea?

dthree commented 8 years ago

Okay I get what you're saying.

That's an interesting bug!

I definitely think it should be improved (that code section), but not removed. It's important that Vorpal knows there's errors and that it's the one that throws them, because sometimes Vorpal calls back to promises, and sometimes callbacks, and it needs to handle them uniformly. Additionally, in Vantage, if the error is thrown on a remote system, it still needs to pipe back the error and display it properly on the user's Node instance, and that wouldn't happen if a regular error were thrown.

Can you see what exactly args.options.book was sending? Maybe Vorpal had some trouble dealing with the datatype.... Kind of weird.

fiatjaf commented 8 years ago

That's a string. A string generated by the Vorpal option parser.

There were other similar code blocks that ended with cb('text ${var}'), and at least one other were causing the same error.

dthree commented 8 years ago

Okay yeah, that's weird!