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

Support YAML config #63

Closed
fregante opened this issue Sep 26, 2016 · 11 comments
Closed

Support YAML config #63

fregante opened this issue Sep 26, 2016 · 11 comments

Comments

@fregante
Copy link
Contributor

fregante commented Sep 26, 2016

It'd be nice if p-s supported YAML configuration alongside the JS one

CommonJS files are slightly easier to read/write than JSON, but JS objects are still full '{"\,}

module.exports = {
  scripts: {
    default: 'node index.js',
    lint: 'eslint .',
    test: {
      // learn more about Jest here: https://kcd.im/egghead-jest
      default: 'jest',
      watch: {
        script: 'jest --watch',
        description: 'run in the amazingly intelligent Jest watch mode'
      }
    },
    build: {
      default: 'webpack',
      prod: 'webpack -p',
    },
    validate: 'nps --parallel lint,test,build',
  },
}

in YAML it would just be

default: node index.js
lint: eslint .
test:
  # learn more about Jest here: https://kcd.im/egghead-jest
  default: jest
  watch:
    script: jest --watch
    description: run in the amazingly intelligent Jest watch mode
build:
  default: webpack
  prod: webpack -p
validate: nps --parallel lint,test,build

Plus, even the syntax highlighter is nicer to YAML :P

YAML isn't dynamic like JS, but probably most people don't use that (and if they do, they can just use the standard CJS syntax)

@kentcdodds
Copy link
Collaborator

I'd be ok with that. Mind filing a PR?

@fregante
Copy link
Contributor Author

I can try, but not this week

@kentcdodds
Copy link
Collaborator

Sounds good to me. Let me know if you need a hand.

@sxn
Copy link
Collaborator

sxn commented Sep 26, 2016

I may have time to look into it this week too, if needed. 😄

@kentcdodds
Copy link
Collaborator

I'm sure that @bfred-it would appreciate it :) It's unlikely that I'll work on this.

sxn added a commit that referenced this issue Sep 30, 2016
* feat(cli): add .yml config support

Allow configuring nps in `package-scripts.yml` instead of `package-scripts.js`

issue: #63

* feat(cli): fix some failing tests

fix some of the tests broken along with YAML support

issue: #63

* feat(cli): reduce cyclomatic complexity of getPSConfig

reduce cyclomatic complexity of getPSConfig

issue: #63

* feat(cli): change precedence of .yml and .js configs

change precedence of `package-scripts.yml` and `package-scripts.js` so that if a .js file is present

it will be used over the .yml one

issue: #63

* feat(cli): remove `package-scripts.yml`

remove `package-scripts.yml`

close issue: #63
@sxn sxn closed this as completed Sep 30, 2016
@sxn
Copy link
Collaborator

sxn commented Sep 30, 2016

:shipit:

@fregante
Copy link
Contributor Author

Looks great!

I was wonder though why that extra script: level was necessary, that's why I didn't include in my example

@kentcdodds
Copy link
Collaborator

Yeah, it's so you can specify options :)

@fregante
Copy link
Contributor Author

Duh! Didn't even notice that. Thanks!

@sxn
Copy link
Collaborator

sxn commented Sep 30, 2016

Yeah, sorry for doing that without asking! Initially I implemented it without the scripts level, but that felt a bit off, i.e. that it would have forced potential YAML fans to still go with the JS version due to the former being (artificially) limited. :)

As a side note: would it make sense to make it so that the init script can create a package-scripts.yml file too?

@kentcdodds
Copy link
Collaborator

Oh yeah, great idea! Feel free to file an issue for that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants