Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

/bin-utils loadJSConfig() doesn't support package-scripts.js exporting a function #88

Closed
erikfox opened this issue Jan 11, 2017 · 4 comments · Fixed by #98
Closed

/bin-utils loadJSConfig() doesn't support package-scripts.js exporting a function #88

erikfox opened this issue Jan 11, 2017 · 4 comments · Fixed by #98

Comments

@erikfox
Copy link
Contributor

erikfox commented Jan 11, 2017

src/get-scripts-from-config.js allows package-scripts.js to export a function:

import {isPlainObject, isFunction} from 'lodash'

export default getScriptsFromConfig

function getScriptsFromConfig(scripts, input) {
  if (isPlainObject(scripts)) {
    return scripts
  } else if (isFunction(scripts)) {
    return scripts(input)
  }
  return {}
}

but src/bin-utils/index.js loadJSConfig() does not:

const loadJSConfig = getAttemptModuleRequireFn(function onFail(configPath, requirePath) {
  log.error({
    message: colors.red(`Unable to find JS config at "${configPath}". Attempted to require as "${requirePath}"`),
    ref: 'unable-to-find-config',
  })
  return undefined
})

...

function loadConfig(configPath) {
  if (configPath.endsWith('.yml')) {
    return loadYAMLConfig(configPath)
  }

  return loadJSConfig(configPath)
}

So when attempting to run a default script via command line, you get the generic help output with a "no scripts" warning:

// package-scripts.js
exports.scripts = function (input) { ... }
$ nps

  Usage: nps [options]

  Options:

    -h, --help                                  output usage information
    -V, --version                               output the version number
    -s, --silent                                Silent nps output
    -p, --parallel <script-name1,script-name2>  Scripts to run in parallel (comma seprated)
    -c, --config <filepath>                     Config file to use (defaults to nearest package-scripts.yml or package-scripts.js)
    -l, --log-level <level>                     The log level to use (error, warn, info [default])
    -r, --require <module>                      Module to preload

There are no scripts available
@erikfox erikfox changed the title /bin-utils loadJSConfig() doesn't support functions /bin-utils loadJSConfig() doesn't support package-scripts.js exporting a function Jan 11, 2017
@kentcdodds
Copy link
Collaborator

Hmmm... I'm not sure I see where the problem is specifically. Do you have a proposed solution? May be easier to understand what's going on if I see a diff for the fixed version.

@erikfox
Copy link
Contributor Author

erikfox commented Jan 11, 2017

@kentcdodds I would like to see the binaries determine whether the config isPlainObject() or isFunction() and evaluate if isFunction() === true before returning as part of getPSConfig() flow.

That way,

  1. when you run nps or nps default, the evaluated default script will run
  2. when you run nps --help, the evaluated scripts will be listed

@erikfox
Copy link
Contributor Author

erikfox commented Jan 11, 2017

If that makes sense, I don't mind writing implementation.

@kentcdodds
Copy link
Collaborator

Give it a go, I look forward to seeing the implementation :) And don't forget to check the contributing guidelines!

kentcdodds pushed a commit that referenced this issue Feb 8, 2017
**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.
kentcdodds pushed a commit that referenced this issue Feb 8, 2017
**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.
kentcdodds pushed a commit that referenced this issue Feb 9, 2017
**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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants