fix(cli): do not swallow any exceptions thrown by JS config files

[boneskull]

A missing config file will report a warning, but any exception that the config file itself throws will be reported to the user. "Empty" configs are now also considered invalid; e.g. a 0-byte .js file or one exporting an plain empty object.

What: #22

Why: Provides a stack trace to user instead of "cannot find" warning, which is inaccurate

How: First leverage require.resolve to ensure the module exists and is readable. If not, warn. If so, execute. If an exception is thrown, report it.

This also requires configs to be non-empty, as I was testing this by using touch package-scripts.js, which resulted in this lovely exception:

/Volumes/alien/forked/p-s/dist/bin-utils/parser.js:124
    var hasDefaultScript = Boolean(psConfig.scripts.default);
                                                   ^

TypeError: Cannot read property 'default' of undefined
    at showHelp (/Volumes/alien/forked/p-s/dist/bin-utils/parser.js:124:52)
    at parse (/Volumes/alien/forked/p-s/dist/bin-utils/parser.js:110:7)
    at Object.<anonymous> (/Volumes/alien/forked/p-s/dist/bin/nps.js:22:33)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:427:7)

Every module loaded by Node has an exports object; if you happen to have a 0-byte .js file, the exports object still exists, but there's nothing in it, and certainly not a scripts object--thus, the above exception.

With my changes, the above won't happen; nps will refer the user to this explanation, which has been updated to require non-empty configs.

(I also note that nps should make a fuss if you are exporting an object, but said object lacks a scripts property. This will likely result in a similar exception, and I have not addressed that case. I can also move this particular change into a new PR or commit if you wish)

[boneskull]

This was actually a bit more involved than I had anticipated--the onFail callback which getAttemptModuleRequireFn() wants is awkward to use if there are multiple paths to failure. Maybe it should just use Promises instead?

[boneskull]

I must also note there's something odd going on, because npm test passes locally.

[boneskull]

Oh, it's the snapshot stuff. Yeah, so that changed, because the only case in which you'd ever see that message is if the module couldn't be resolved by require.resolve(), and if that's true, there's no "Attempted to require as..." b/c we didn't get that far.

As a contributor, I'm not sure what I'm supposed to do about testing against snapshots I don't have on my local machine. IMO, this snapshot failure should be a warning, because otherwise it's crying wolf. Don't know if this is even possible with Travis CI.

[kentcdodds]

I think you need to update snapshots for the CLI tests. You can do this with: npm start "test.cli -u"

[boneskull]

@kentcdodds Ahh, thanks. Didn't know they were in VCS. Updated & pusht

[codecov-io]

Codecov Report

Merging #140 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #140   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         308    315    +7     
=====================================
+ Hits          308    315    +7

Impacted Files	Coverage Δ
src/bin-utils/index.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 76bee57...9e4d808. Read the comment docs.

[boneskull]

Some of the extra info from @codecov-io is cool, but this is a bit nonsensical:

[kentcdodds]

Yeah, it's more useful when you don't have 100% code coverage. I pretty much ignore the bot 🙃 thanks a bunch!