Yargs crashes when an invalid package.json is the directory tree.

[avribacki]

I'm bundling my application using nexe and it works pretty well except for one particular case.
When there's an invalid package.json along the directory from where I'm executing my application, Yargs crashes. If the package.json is valid, it works as normal.

For instance, if try to run "myapp -h" and there's an empty package.json in the same folder, I get the following error:

nexe.js:102738
                throw jsonErr;
                ^
 JSONError: No data, empty input at 1:1 in package.json
^
    at module.exports (nexe.js:102732:17)
    at parse (nexe.js:102684:9)
    at Function.module.exports.sync (nexe.js:102694:9)
    at Function.module.exports.sync (nexe.js:102580:25)
    at parseArgs (nexe.js:107341:37)
    at Object.Yargs.self.showHelp (nexe.js:107192:23)
    at Object.self.fail (nexe.js:99757:35)
    at Object.Yargs.Object.defineProperty.get [as argv] (nexe.js:107331:15)
    at Object.__dirname.1../lib/download.js (nexe.js:48:5)
    at s (nexe.js:1:316)

This might be a corner case and most likely to happen only when yargs is bundled. Otherwise, it will try to use the package.json of the node package and it will likely be OK.

Once bundled, it can run from any location and there might be an invalid package.json lurking around which will produce this crash, which was not obvious to guess the origin.

Yargs seems a perfect fit for command line tools bundled with nexe, that's why I think this deserves a fix.

When I protected the pkgConfig.sync inside parseArgs method, it no longer crashed:

    try {
      options.configuration = pkgConf.sync('yargs', {
        defaults: {},
        cwd: requireMainFilename(require)
      })
    }
    catch (err) {
      options.configuration = {}
    }

Not sure if this is the best solution. I think that the ideal would be to have an option to disable this pkgConf.sync call as, in the context I presented, I don't expect that some package.json lost in my file system to change the app behavior.

[nexdrew]

@avribacki Thanks for reporting this! I can confirm this is a potential problem, and it's not limited to when yargs is bundled.

I think adding something to the yargs API to disable the package.json lookup is a good idea. When doing so, we should probably invent another way of specifying the yargs-parser configuration options, possibly by accepting a "parserConfig" object that yargs can pass directly to yargs-parser. Would be nice to accomplish both of these with the same API.

Thoughts (from anyone)?

[maxrimue]

@nexdrew @avribacki
I'm not entirely sure about this. While I do think it is pretty unlikely for yargs to run while the main package.json is empty, it seems kinda weird that yargs simply crashes because of that. It also crashes if the package.json is holding incorrect JSON code, but that seems like a nice way to find out you need to correct your JSON code.

Since package.json data never really is mandatory for yargs to work (it could live without that data), I'm simply not sure whether this is a bug to fix or a new config option to add. Thoughts?

What I do like is @nexdrew's idea for letting parser options be passed through yargs' configuration, as it currently is only possible to set parser options in the package.json, not in code.

[avribacki]

I think that it would be much more valuable to add a new way of passing the parser options (avoiding the search for the package.json entirely) than fixing the crash.
If you expect the options to be inside package.json, then ignoring it silently as I did on the issue description is a bad idea.
Couldn't it be a just a new method that would be chained through yargs as all the other configurations? This would also be backward compatible as if the options were not supplied, they would still be looked-up in package.json as it is today.

[nexdrew]

@avribacki @maxrimue Yes, I agree. I think this should be a new API method, as you describe. If parser config is given explicitly via the new API method, then yargs should forgo the package.json lookup; otherwise, keep the current functionality and allow a crash on invalid package.json (has to be the first/nearest one found when traversing up parent directories).

[bcoe]

a couple thoughts:

1. I agree with @nexdrew, we let's make it so that package lookups can be disabled in yargs-parser.
2. @elas7 just introduced functionality that allows configuration to be passed an arbitrary object (feat(configuration): Allow to directly pass a configuration object to .config() #480), I think this addresses some of the ideas discussed in this issue.
3. I think that rather than crashing if an invalid package.json is found, we should instead console.warn() -- this seems like something we should put a test around.

[bcoe]

@avribacki give this a spin:

npm cache clear; npm i yargs@next

should no longer be crashing on you.