Conversation
|
@JaKXz you were mentioning you'd be happy to start helping out on yargs as well? Would love the extra set of 👀 on this. |
| for (var i = 0, ii = args.length; i < ii; ++i) { | ||
| if (handlers[args[i]] && handlers[args[i]].builder) { | ||
| return handlers[args[i]].builder(yargs.reset()).argv | ||
| const builder = handlers[args[i]].builder |
There was a problem hiding this comment.
this logic was creating an exception that we were quietly ignoring. It came up when the builder was an object rather than a function, or the builder returned nothing -- because this error wasn't a YError tests started failing.
There was a problem hiding this comment.
So this isn't actually related to YError but a bug that was discovered because of it?
| describe: 'second option' | ||
| } | ||
| }) | ||
| .argv |
There was a problem hiding this comment.
this test wasn't using the command api appropriately.
| function YError (msg) { | ||
| this.name = 'YError' | ||
| this.message = msg || 'yargs error' | ||
| this.stack = (new Error()).stack |
There was a problem hiding this comment.
I think this last line would be better as:
Error.captureStackTrace(this, YError)
By using captureStackTrace you won't get the YError constructor in the call stack.
| args = parseArgs(processArgs) | ||
| } catch (err) { | ||
| usage.fail(err.message, err) | ||
| if (err instanceof YError) usage.fail(err.message, err) |
There was a problem hiding this comment.
Ah, so this is the magic moment!
| @@ -0,0 +1,11 @@ | |||
| const util = require('util') | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
That's a lotta code to do a very little thing… Ben's implementation is 9 lines (a third of which are whitespace) of very simple js, versus … 62 lines. Now concise isn't automatically better, but personally I find what's here way easier to grok. A lot of the weirdness in those 62 lines are about browser compatibility, is that a thing that yargs cares about?
There was a problem hiding this comment.
@satazor @iarna hadn't thought of the front-end use-case; I know in the past a couple people had used something like browserify to load yargs in the browser. It might be worth switching from util.inherits to something like:
MyError.prototype = Object.create(Error.prototype);
MyError.prototype.constructor = MyError;Which I lifted rom the MDN docs.
@satazor to answer your question; I get a lot of pressure from the user-base to keep yargs to as few dependencies as possible:
More so than a lot of libraries I work on, this has motivated me to opt for less dependencies where possible -- I think people are extra sensitive about library-size, because yargs is something you tack onto a project to add CLI functionality, it's not seen as a core part of the application logic.
There was a problem hiding this comment.
This was my only nitpick, thanks for bringing it up @satazor - here's the nodejs discussion on the discouragement of using util.inherits: nodejs/node#4179
| args = parseArgs(processArgs) | ||
| } catch (err) { | ||
| usage.fail(err.message, err) | ||
| if (err instanceof YError) usage.fail(err.message, err) |
BREAKING CHANGE: yargs will no longer aggressively supress errors, allowing errors that are not generated internally to bubble.
BREAKING CHANGE: yargs will no longer aggressively supress errors, allowing errors that are not generated internally to bubble.
BREAKING CHANGE: yargs will no longer aggressively supress errors, allowing errors that are not generated internally to bubble.
4 participants
BREAKING CHANGE: yargs will no longer aggressively supress errors, allowing errors that are not generated internally to bubble.
CC: @iarna, @satazor curious as to what you think of this implementation.