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

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.

#22

.all-contributorsrc

@@ -118,7 +118,11 @@
       "avatar_url": "https://avatars.githubusercontent.com/u/924465?v=3",
       "profile": "https://boneskull.com",
       "contributions": [
-        "review"
+        "review",
+        "bug",
+        "code",
+        "doc",
+        "test"
       ]
     },
     {

README.md

@@ -435,7 +435,7 @@ Thanks goes to these people ([emoji key][emojis]):
 <!-- ALL-CONTRIBUTORS-LIST:START - Do not remove or modify this section -->
 | [<img src="https://avatars.githubusercontent.com/u/1500684?v=3" width="100px;"/><br /><sub>Kent C. Dodds</sub>](http://kent.doddsfamily.us)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=kentcdodds) [📖](https://github.com/kentcdodds/p-s/commits?author=kentcdodds) 🚇 💡 📹 👀 | [<img src="https://avatars.githubusercontent.com/u/532272?v=3" width="100px;"/><br /><sub>David Wells</sub>](http://davidwells.io)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=DavidWells) | [<img src="https://avatars.githubusercontent.com/u/802242?v=3" width="100px;"/><br /><sub>Abhishek Shende</sub>](https://twitter.com/abhishekisnot)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=abhishekisnot) [⚠️](https://github.com/kentcdodds/p-s/commits?author=abhishekisnot) | [<img src="https://avatars.githubusercontent.com/u/185649?v=3" width="100px;"/><br /><sub>Rowan Oulton</sub>](http://travelog.io)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=rowanoulton) [📖](https://github.com/kentcdodds/p-s/commits?author=rowanoulton) [⚠️](https://github.com/kentcdodds/p-s/commits?author=rowanoulton) | [<img src="https://avatars.githubusercontent.com/u/1915716?v=3" width="100px;"/><br /><sub>Gilad Goldberg</sub>](https://github.com/giladgo)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=giladgo) | [<img src="https://avatars.githubusercontent.com/u/14267457?v=3" width="100px;"/><br /><sub>Tim McGee</sub>](https://github.com/tim-mcgee)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=tim-mcgee) [📖](https://github.com/kentcdodds/p-s/commits?author=tim-mcgee) | [<img src="https://avatars.githubusercontent.com/u/175264?v=3" width="100px;"/><br /><sub>Nik Butenko</sub>](http://butenko.me)<br />💡 [💻](https://github.com/kentcdodds/p-s/commits?author=nkbt) |
 | :---: | :---: | :---: | :---: | :---: | :---: | :---: |
-| [<img src="https://avatars.githubusercontent.com/u/1972567?v=3" width="100px;"/><br /><sub>Tommy</sub>](http://www.tommyleunen.com)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Atleunen) [💻](https://github.com/kentcdodds/p-s/commits?author=tleunen) [⚠️](https://github.com/kentcdodds/p-s/commits?author=tleunen) 👀 | [<img src="https://avatars.githubusercontent.com/u/509946?v=3" width="100px;"/><br /><sub>Jayson Harshbarger</sub>](http://www.hypercubed.com)<br />💡 👀 | [<img src="https://avatars.githubusercontent.com/u/1355481?v=3" width="100px;"/><br /><sub>JD Isaacks</sub>](http://www.jisaacks.com)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=jisaacks) [⚠️](https://github.com/kentcdodds/p-s/commits?author=jisaacks) | [<img src="https://avatars.githubusercontent.com/u/924465?v=3" width="100px;"/><br /><sub>Christopher Hiller</sub>](https://boneskull.com)<br />👀 | [<img src="https://avatars.githubusercontent.com/u/1834413?v=3" width="100px;"/><br /><sub>Robin Malfait</sub>](https://robinmalfait.com)<br />💡 | [<img src="https://avatars.githubusercontent.com/u/622118?v=3" width="100px;"/><br /><sub>Eric McCormick</sub>](https://ericmccormick.io)<br />👀 [📖](https://github.com/kentcdodds/p-s/commits?author=edm00se) | [<img src="https://avatars.githubusercontent.com/u/1913805?v=3" width="100px;"/><br /><sub>Sam Verschueren</sub>](https://twitter.com/SamVerschueren)<br />👀 |
+| [<img src="https://avatars.githubusercontent.com/u/1972567?v=3" width="100px;"/><br /><sub>Tommy</sub>](http://www.tommyleunen.com)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Atleunen) [💻](https://github.com/kentcdodds/p-s/commits?author=tleunen) [⚠️](https://github.com/kentcdodds/p-s/commits?author=tleunen) 👀 | [<img src="https://avatars.githubusercontent.com/u/509946?v=3" width="100px;"/><br /><sub>Jayson Harshbarger</sub>](http://www.hypercubed.com)<br />💡 👀 | [<img src="https://avatars.githubusercontent.com/u/1355481?v=3" width="100px;"/><br /><sub>JD Isaacks</sub>](http://www.jisaacks.com)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=jisaacks) [⚠️](https://github.com/kentcdodds/p-s/commits?author=jisaacks) | [<img src="https://avatars.githubusercontent.com/u/924465?v=3" width="100px;"/><br /><sub>Christopher Hiller</sub>](https://boneskull.com)<br />👀 [🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Aboneskull) [💻](https://github.com/kentcdodds/p-s/commits?author=boneskull) [📖](https://github.com/kentcdodds/p-s/commits?author=boneskull) [⚠️](https://github.com/kentcdodds/p-s/commits?author=boneskull) | [<img src="https://avatars.githubusercontent.com/u/1834413?v=3" width="100px;"/><br /><sub>Robin Malfait</sub>](https://robinmalfait.com)<br />💡 | [<img src="https://avatars.githubusercontent.com/u/622118?v=3" width="100px;"/><br /><sub>Eric McCormick</sub>](https://ericmccormick.io)<br />👀 [📖](https://github.com/kentcdodds/p-s/commits?author=edm00se) | [<img src="https://avatars.githubusercontent.com/u/1913805?v=3" width="100px;"/><br /><sub>Sam Verschueren</sub>](https://twitter.com/SamVerschueren)<br />👀 |
 | [<img src="https://avatars.githubusercontent.com/u/1155589?v=3" width="100px;"/><br /><sub>Sorin Muntean</sub>](https://github.com/sxn)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=sxn) [⚠️](https://github.com/kentcdodds/p-s/commits?author=sxn) [📖](https://github.com/kentcdodds/p-s/commits?author=sxn) | [<img src="https://avatars.githubusercontent.com/u/1970063?v=3" width="100px;"/><br /><sub>Keith Gunn</sub>](https://github.com/gunnx)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Agunnx) [💻](https://github.com/kentcdodds/p-s/commits?author=gunnx) [⚠️](https://github.com/kentcdodds/p-s/commits?author=gunnx) | [<img src="https://avatars.githubusercontent.com/u/1019478?v=3" width="100px;"/><br /><sub>Joe Martella</sub>](http://martellaj.github.io)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Amartellaj) [💻](https://github.com/kentcdodds/p-s/commits?author=martellaj) [⚠️](https://github.com/kentcdodds/p-s/commits?author=martellaj) | [<img src="https://avatars.githubusercontent.com/u/1887854?v=3" width="100px;"/><br /><sub>Martin Segado</sub>](https://github.com/msegado)<br />[📖](https://github.com/kentcdodds/p-s/commits?author=msegado) | [<img src="https://avatars.githubusercontent.com/u/36491?v=3" width="100px;"/><br /><sub>Bram Borggreve</sub>](http://colmena.io/)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Abeeman) [💻](https://github.com/kentcdodds/p-s/commits?author=beeman) | [<img src="https://avatars.githubusercontent.com/u/86454?v=3" width="100px;"/><br /><sub>Elijah Manor</sub>](http://elijahmanor.com)<br />📹 | [<img src="https://avatars.githubusercontent.com/u/10691183?v=3" width="100px;"/><br /><sub>Ragu Ramaswamy</sub>](https://github.com/rrag)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=rrag) [⚠️](https://github.com/kentcdodds/p-s/commits?author=rrag) [🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Arrag) |
 | [<img src="https://avatars.githubusercontent.com/u/2915616?v=3" width="100px;"/><br /><sub>Erik Fox</sub>](http://www.erikfox.co/)<br />[🐛](https://github.com/kentcdodds/p-s/issues?q=author%3Aerikfox) | [<img src="https://avatars.githubusercontent.com/u/5351262?v=3" width="100px;"/><br /><sub>Aditya Pratap Singh</sub>](http://blog.adityapsingh.com)<br />👀 | [<img src="https://avatars.githubusercontent.com/u/7687132?v=3" width="100px;"/><br /><sub>bumbleblym</sub>](https://github.com/bumbleblym)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=bumbleblym) [📖](https://github.com/kentcdodds/p-s/commits?author=bumbleblym) | [<img src="https://avatars.githubusercontent.com/u/7091543?v=3" width="100px;"/><br /><sub>Islam Attrash</sub>](https://twitter.com/IslamAttrash)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=Attrash-Islam) | [<img src="https://avatars.githubusercontent.com/u/7215306?v=3" width="100px;"/><br /><sub>JasonSooter</sub>](https://github.com/JasonSooter)<br />[📖](https://github.com/kentcdodds/p-s/commits?author=JasonSooter) | [<img src="https://avatars1.githubusercontent.com/u/116871?v=3" width="100px;"/><br /><sub>Nate Cavanaugh</sub>](http://alterform.com)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=natecavanaugh) | [<img src="https://avatars2.githubusercontent.com/u/3534924?v=3" width="100px;"/><br /><sub>Wissam Abirached</sub>](https://designingforscale.com)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=wabirached) [⚠️](https://github.com/kentcdodds/p-s/commits?author=wabirached) |
 | [<img src="https://avatars1.githubusercontent.com/u/12592677?v=3" width="100px;"/><br /><sub>Paweł Mikołajczyk</sub>](https://github.com/Miklet)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=Miklet) [⚠️](https://github.com/kentcdodds/p-s/commits?author=Miklet) | [<img src="https://avatars0.githubusercontent.com/u/1295580?v=3" width="100px;"/><br /><sub>Kyle Welch</sub>](http://www.krwelch.com)<br />[💻](https://github.com/kentcdodds/p-s/commits?author=kwelch) [⚠️](https://github.com/kentcdodds/p-s/commits?author=kwelch) |

cli-test/__snapshots__/cli.test.js.snap

@@ -28,7 +28,7 @@ Object {
 
 exports[`test with a missing config 1`] = `
 Object {
-  "stderr": "Unable to find JS config at \"./something-that-does-not-exist.js\". Attempted to require as \"<projectRootDir>/cli-test/fixtures/something-that-does-not-exist.js\" https://github.com/kentcdodds/nps/blob/v0.0.0-semantically-released/other/ERRORS_AND_WARNINGS.md#unable-to-find-config
+  "stderr": "Unable to find JS config at \"./something-that-does-not-exist.js\". https://github.com/kentcdodds/nps/blob/v0.0.0-semantically-released/other/ERRORS_AND_WARNINGS.md#unable-to-find-config
 ",
   "stdout": "",
 }

other/ERRORS_AND_WARNINGS.md

@@ -48,7 +48,7 @@ Try to run the script without `nps` and verify that the script is working. If no
 
 ## Config Must be an Object
 
-Your `package-scripts.js`, `package-scripts.yml`, or whatever you specified as the `--config` must be an object or a function that returns an object.
+Your `package-scripts.js`, `package-scripts.yml`, or whatever you specified as the `--config` must be a nopn-empty object or a function that returns a non-empty object.
 
 ### To Fix:
 

src/bin-utils/fixtures/bad-empty-module.js

@@ -0,0 +1 @@
+// crickets

src/bin-utils/fixtures/bad-function-empty-module.js

@@ -0,0 +1,3 @@
+module.exports = function () {
+  return {}
+}

src/bin-utils/fixtures/other-error-module.js

@@ -0,0 +1,6 @@
+module.exports = {
+  foo: 'Hello',
+  bar: 'No comma!'
+};
+
+throw new Error('oops, I did it again')

src/bin-utils/index.js

@@ -1,6 +1,12 @@
 import {resolve} from 'path'
 import {readFileSync} from 'fs'
-import {includes, isPlainObject, isUndefined, isFunction} from 'lodash'
+import {
+  includes,
+  isPlainObject,
+  isUndefined,
+  isEmpty,
+  isFunction,
+} from 'lodash'
 import typeOf from 'type-detect'
 import chalk from 'chalk'
 import {safeLoad} from 'js-yaml'
@@ -33,12 +39,15 @@ const preloadModule = getAttemptModuleRequireFn((moduleName, requirePath) => {
 const loadJSConfig = getAttemptModuleRequireFn(function onFail(
   configPath,
   requirePath,
+  err,
 ) {
+  if (err) {
+    throw err
+  }
   log.error({
     message: chalk.red(
       oneLine`
         Unable to find JS config at "${configPath}".
-        Attempted to require as "${requirePath}"
       `,
     ),
     ref: 'unable-to-find-config',
@@ -62,23 +71,25 @@ function loadConfig(configPath, input) {
     // let the caller deal with this
     return config
   }
-  let typeMessage
+  let typeMessage = `Your config data type was`
   if (isFunction(config)) {
     config = config(input)
-    typeMessage = oneLine`
-      Your config was a function that returned
-      a data type of "${typeOf(config)}"
-    `
+    typeMessage = `${typeMessage} a function which returned`
+  }
+  const emptyConfig = isEmpty(config)
+  const plainObjectConfig = isPlainObject(config)
+  if (plainObjectConfig && emptyConfig) {
+    typeMessage = `${typeMessage} an object, but it was empty`
   } else {
-    typeMessage = `Your config data type was "${typeOf(config)}"`
+    typeMessage = `${typeMessage} a data type of "${typeOf(config)}"`
   }
-  if (!isPlainObject(config)) {
+  if (!plainObjectConfig || emptyConfig) {
     log.error({
       message: chalk.red(
         oneLine`
           The package-scripts configuration
-          ("${configPath.replace(/\\/g, '/')}") must be an object
-          or a function that returns an object.
+          ("${configPath.replace(/\\/g, '/')}") must be a non-empty object
+          or a function that returns a non-empty object.
         `,
       ),
       ref: 'config-must-be-an-object',
@@ -115,20 +126,22 @@ function loadYAMLConfig(configPath) {
  */
 function getModuleRequirePath(moduleName) {
   return moduleName[0] === '.' ?
-    resolve(process.cwd(), moduleName) :
+    require.resolve(resolve(process.cwd(), moduleName)) :
     moduleName
 }
 
 function getAttemptModuleRequireFn(onFail) {
   return function attemptModuleRequire(moduleName) {
-    const requirePath = getModuleRequirePath(moduleName)
+    let requirePath
+    try {
+      requirePath = getModuleRequirePath(moduleName)
+    } catch (e) {
+      return onFail(moduleName)
+    }
     try {
       return requireDefaultFromModule(requirePath)
     } catch (e) {
-      if (e.constructor.name === 'SyntaxError') {
-        throw e
-      }
-      return onFail(moduleName, requirePath)
+      return onFail(moduleName, requirePath, e)
     }
   }
 }

src/bin-utils/index.test.js

@@ -71,6 +71,31 @@ test(
   },
 )
 
+test(
+  oneLine`
+    loadConfig: logs and throws an error
+    for a config that exports an empty config
+  `,
+  () => {
+    const mockError = jest.fn()
+    jest.resetModules()
+    jest.mock('../get-logger', () => () => ({error: mockError}))
+    const {loadConfig: proxiedLoadConfig} = require('./index')
+    const fixturePath = getAbsoluteFixturePath('bad-empty-module.js').replace(
+      /\\/g,
+      '/',
+    )
+    expect(() => proxiedLoadConfig(fixturePath)).toThrowError(
+      /Your config data type was an object, but it was empty/,
+    )
+    expect(mockError).toHaveBeenCalledTimes(1)
+    expect(mockError).toHaveBeenCalledWith({
+      message: expect.stringMatching(fixturePath),
+      ref: 'config-must-be-an-object',
+    })
+  },
+)
+
 test(
   oneLine`
     loadConfig: logs and throws an error for a
@@ -96,6 +121,31 @@ test(
   },
 )
 
+test(
+  oneLine`
+    loadConfig: logs and throws an error for a
+    config that exports a function that returns
+    an empty config
+  `,
+  () => {
+    const mockError = jest.fn()
+    jest.resetModules()
+    jest.mock('../get-logger', () => () => ({error: mockError}))
+    const {loadConfig: proxiedLoadConfig} = require('./index')
+    const fixturePath = getAbsoluteFixturePath(
+      'bad-function-empty-module.js',
+    ).replace(/\\/g, '/')
+    expect(() => proxiedLoadConfig(fixturePath)).toThrowError(
+      /Your config.*function which returned an object, but it was empty/,
+    )
+    expect(mockError).toHaveBeenCalledTimes(1)
+    expect(mockError).toHaveBeenCalledWith({
+      message: expect.stringMatching(fixturePath),
+      ref: 'config-must-be-an-object',
+    })
+  },
+)
+
 test('loadConfig: logs a warning when the JS module cannot be required', () => {
   const mockError = spy()
   jest.resetModules()
@@ -118,6 +168,14 @@ test('loadConfig: does not swallow JS syntax errors', () => {
   process.cwd = originalCwd
 })
 
+test('loadConfig: does not swallow other thrown JS exceptions', () => {
+  const originalCwd = process.cwd
+  process.cwd = jest.fn(() => path.resolve(__dirname, '../..'))
+  const relativePath = './src/bin-utils/fixtures/other-error-module'
+  expect(() => loadConfig(relativePath)).toThrowError()
+  process.cwd = originalCwd
+})
+
 test('loadConfig: can load ES6 module', () => {
   const relativePath = './src/bin-utils/fixtures/fake-es6-module'
   const val = loadConfig(relativePath)