fix(load): allow function to be exported (as docs say) (#98)

**What**: closes #88

**Why**: Because we document that you can export a function that returns
a configuration, but it doesn't actually work.

**How**: Add some logic to `loadConfig` in `src/bin-utils/index.js` to
handle the case when the config is a function. Also, we need to pass the
input along, so I had to make a few other changes as well. Also added
fixtures and tests for this.

.all-contributorsrc

@@ -219,6 +219,15 @@
         "code",
         "test"
       ]
+    },
+    {
+      "login": "erikfox",
+      "name": "Erik Fox",
+      "avatar_url": "https://avatars.githubusercontent.com/u/2915616?v=3",
+      "profile": "http://www.erikfox.co/",
+      "contributions": [
+        "bug"
+      ]
     }
   ]
 }

README.md

@@ -11,7 +11,7 @@ All the benefits of npm scripts without the cost of a bloated package.json and l
 [![downloads][downloads-badge]][npm-stat]
 [![MIT License][license-badge]][LICENSE]
 
-[![All Contributors](https://img.shields.io/badge/all_contributors-21-orange.svg?style=flat-square)](#contributors)
+[![All Contributors](https://img.shields.io/badge/all_contributors-22-orange.svg?style=flat-square)](#contributors)
 [![PRs Welcome][prs-badge]][prs]
 [![Donate][donate-badge]][donate]
 [![Code of Conduct][coc-badge]][coc]
@@ -431,6 +431,7 @@ Thanks goes to these people ([emoji key][emojis]):
 | :---: | :---: | :---: | :---: | :---: | :---: | :---: |
 | [<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/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) |
+| [<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) |
 <!-- ALL-CONTRIBUTORS-LIST:END -->
 
 This project follows the [all-contributors][all-contributors] specification.

other/ERRORS_AND_WARNINGS.md

@@ -45,3 +45,11 @@ This means that the child process for the specified script emitted an error.
 Look at the error and try to figure out why the script would be failing.
 
 Try to run the script without `p-s` and verify that the script is working. If not, fix that. If it's working without `p-s` it could be a problem with `p-s`. Please file an issue.
+
+## 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.
+
+### To Fix:
+
+Make sure that your config is an object or a function that returns an object.

package.json

@@ -33,7 +33,8 @@
     "prefix-matches": "^0.0.9",
     "readline-sync": "^1.4.6",
     "shell-escape": "^0.2.0",
-    "spawn-command-with-kill": "^1.0.0"
+    "spawn-command-with-kill": "^1.0.0",
+    "type-detect": "^4.0.0"
   },
   "devDependencies": {
     "all-contributors-cli": "^3.1.0",

src/bin-utils/fixtures/bad-data-type-config.js

@@ -0,0 +1 @@
+module.exports = 'cannot be a string'

src/bin-utils/fixtures/bad-function-data-type-config.js

@@ -0,0 +1 @@
+module.exports = () => ['cannot be a function that returns an array']

src/bin-utils/fixtures/function-config.js

@@ -0,0 +1,5 @@
+module.exports = () => ({
+  scripts: {
+    functionConfig: 'echo worked!',
+  }
+})

src/bin-utils/index.js

@@ -1,6 +1,7 @@
 import {resolve} from 'path'
 import {readFileSync} from 'fs'
-import {remove, includes, isPlainObject, isEmpty} from 'lodash'
+import {remove, includes, isPlainObject, isUndefined, isEmpty, isFunction} from 'lodash'
+import typeOf from 'type-detect'
 import shellEscape from 'shell-escape'
 import colors from 'colors/safe'
 import {safeLoad} from 'js-yaml'
@@ -35,15 +36,38 @@ const loadJSConfig = getAttemptModuleRequireFn(function onFail(configPath, requi
 
 /**
  * Attempts to load the config and logs an error if there's a problem
- * @param  {String} configPath The path to attempt to require the config from
- * @return {*} The required module
+ * @param {String} configPath The path to attempt to require the config from
+ * @param {*} input the input to pass to the config if it's a function
+ * @return {Object} The config
  */
-function loadConfig(configPath) {
+function loadConfig(configPath, input) {
   if (configPath.endsWith('.yml')) {
     return loadYAMLConfig(configPath)
   }
 
-  return loadJSConfig(configPath)
+  let config = loadJSConfig(configPath)
+  if (isUndefined(config)) {
+    // let the caller deal with this
+    return config
+  }
+  let typeMessage
+  if (isFunction(config)) {
+    config = config(input)
+    typeMessage = `Your config was a function that returned a data type of "${typeOf(config)}"`
+  } else {
+    typeMessage = `Your config data type was "${typeOf(config)}"`
+  }
+  if (!isPlainObject(config)) {
+    log.error({
+      message: colors.red(
+        `The package-scripts configuration ("${configPath}") ` +
+        'must be an object or a function that returns an object.',
+      ),
+      ref: 'config-must-be-an-object',
+    })
+    throw new Error(typeMessage)
+  }
+  return config
 }
 
 export {

src/bin-utils/index.test.js

@@ -1,5 +1,5 @@
 /* eslint import/newline-after-import:0, global-require:0 */
-import {resolve} from 'path'
+import path from 'path'
 import colors from 'colors/safe'
 import {spy} from 'sinon'
 import {getScriptsAndArgs, help, preloadModule, loadConfig} from './index'
@@ -56,8 +56,7 @@ test('preloadModule: resolves a relative path', () => {
 })
 
 test('preloadModule: resolves an absolute path', () => {
-  const relativePath = './fixtures/my-module'
-  const absolutePath = resolve(__dirname, relativePath)
+  const absolutePath = getAbsoluteFixturePath('my-module')
   const val = preloadModule(absolutePath)
   expect(val).toBe('hello')
 })
@@ -74,11 +73,50 @@ test('preloadModule: logs a warning when the module cannot be required', () => {
   const {preloadModule: proxiedPreloadModule} = require('./index')
   const val = proxiedPreloadModule('./module-that-does-exist')
   expect(val).toBeUndefined()
-  expect(mockWarn.calledOnce)
+  expect(mockWarn.calledOnce).toBe(true)
   const [{message}] = mockWarn.firstCall.args
   expect(message).toMatch(/Unable to preload "\.\/module-that-does-exist"/)
 })
 
+test('loadConfig: calls the config function if it is a function', () => {
+  jest.resetModules()
+  const {loadConfig: proxiedLoadConfig} = require('./index')
+  const val = proxiedLoadConfig(getAbsoluteFixturePath('function-config.js'))
+  expect(val).toEqual({
+    scripts: {
+      functionConfig: 'echo worked!',
+    },
+  })
+})
+
+test('loadConfig: logs and throws an error for a config that exports the wrong data type', () => {
+  const mockError = jest.fn()
+  jest.resetModules()
+  jest.mock('../get-logger', () => () => ({error: mockError}))
+  const {loadConfig: proxiedLoadConfig} = require('./index')
+  const fixturePath = getAbsoluteFixturePath('bad-data-type-config.js')
+  expect(() => proxiedLoadConfig(fixturePath)).toThrowError(/Your config.*string/)
+  expect(mockError).toHaveBeenCalledTimes(1)
+  expect(mockError).toHaveBeenCalledWith({
+    message: expect.stringMatching(fixturePath),
+    ref: 'config-must-be-an-object',
+  })
+})
+
+test('loadConfig: logs and throws an error for a config that exports a function that returns the wrong data type', () => {
+  const mockError = jest.fn()
+  jest.resetModules()
+  jest.mock('../get-logger', () => () => ({error: mockError}))
+  const {loadConfig: proxiedLoadConfig} = require('./index')
+  const fixturePath = getAbsoluteFixturePath('bad-function-data-type-config.js')
+  expect(() => proxiedLoadConfig(fixturePath)).toThrowError(/Your config.*function.*Array/)
+  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()
@@ -93,7 +131,7 @@ test('loadConfig: logs a warning when the JS module cannot be required', () => {
 
 test('loadConfig: does not swallow JS syntax errors', () => {
   const originalCwd = process.cwd
-  process.cwd = jest.fn(() => resolve(__dirname, '../..'))
+  process.cwd = jest.fn(() => path.resolve(__dirname, '../..'))
   const relativePath = './src/bin-utils/fixtures/syntax-error-module'
   expect(() => loadConfig(relativePath)).toThrowError()
   process.cwd = originalCwd
@@ -112,7 +150,7 @@ test('loadConfig: can load ES6 module', () => {
 
 test('loadConfig: does not swallow YAML syntax errors', () => {
   const originalCwd = process.cwd
-  process.cwd = jest.fn(() => resolve(__dirname, '../..'))
+  process.cwd = jest.fn(() => path.resolve(__dirname, '../..'))
   const relativePath = './src/bin-utils/fixtures/syntax-error-config.yml'
   expect(() => loadConfig(relativePath)).toThrowError()
   process.cwd = originalCwd
@@ -217,3 +255,7 @@ test('help: do not display scripts with flag hiddenFromHelp set to true', () =>
   const expected = colors.yellow('There are no scripts available')
   expect(message).toBe(expected)
 })
+
+function getAbsoluteFixturePath(fixture) {
+  return path.join(__dirname, 'fixtures', fixture)
+}

src/bin/nps.js

@@ -32,13 +32,13 @@ program
   .on('--help', onHelp)
   .parse(process.argv)
 
+const scriptsAndArgs = getScriptsAndArgs(program)
 
 if (shouldAutocomplete) {
   autocomplete(getPSConfig())
 } else if (shouldRun) {
   const psConfig = getPSConfig()
   const hasDefaultScript = !!psConfig.scripts.default
-  const scriptsAndArgs = getScriptsAndArgs(program)
   const hasHelpScript = !!psConfig.scripts.help
   const scriptIsHelp = scriptsAndArgs.scripts[0] === 'help'
   const scriptSpecified = scriptsAndArgs.scripts.length >= 1
@@ -47,11 +47,11 @@ if (shouldAutocomplete) {
   } else if (!hasHelpScript && scriptIsHelp) { // eslint-disable-line no-negated-condition
     program.outputHelp()
   } else {
-    loadAndRun(scriptsAndArgs, psConfig)
+    loadAndRun(psConfig)
   }
 }
 
-function loadAndRun(scriptsAndArgs, psConfig) {
+function loadAndRun(psConfig) {
   runPackageScript({
     scriptConfig: psConfig.scripts,
     scripts: scriptsAndArgs.scripts,
@@ -76,7 +76,7 @@ function getPSConfig() {
     log.warn(colors.yellow('Unable to find a config file and none was specified.'))
     return {scripts: {}} // empty config
   }
-  const config = loadConfig(configFilepath)
+  const config = loadConfig(configFilepath, scriptsAndArgs)
   if (!config) {
     process.exit(FAIL_CODE)
   }