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

Methods should use config path defined in package.json #13

Open
ghost opened this issue Mar 26, 2017 · 11 comments
Open

Methods should use config path defined in package.json #13

ghost opened this issue Mar 26, 2017 · 11 comments

Comments

@ghost
Copy link

ghost commented Mar 26, 2017

While refactoring my package.json to use nps I haven noticed that methods like:

  • concurrent.nps or
  • series.nps

ignore custom config path provided via "nps --config". It would be extremely easy to check if env contains config path e.g.
process.argv.slice(3,4)
& reuse it while creating new nps call in following locations (nps-utils/src/index.js):

That would simplify reference of scripts with config in custom location, don't you think?

I'm happy to make pull request but due to #12 I'm not confident that this repo is ready to be forked ;-/

@kentcdodds
Copy link
Owner

This makes sense to me. Let's do it. Could you please respond to my question in #12? Thanks!

@kentcdodds
Copy link
Owner

Note, unfortunately this won't be as simple as calling slice because the options and scripts can appear in any order. We'll need to parse things via yargs.

@ghost
Copy link
Author

ghost commented Mar 29, 2017

@kentcdodds sure - that make sense. If you can check file I have provided for #12 & help with this I can quickly add this feature.

BTW. nps is such an amazing tool - showed that to mates from my team & they were impressed how easy you can clean package.json xD

@kentcdodds
Copy link
Owner

I'm glad you like it! I really enjoy it as well :)

@RichDonnellan
Copy link

RichDonnellan commented Jun 1, 2017

@kentcdodds Looks like the @ghost account has been deleted. I'm facing the same dilemma and can't seem to set the custom --config within these methods in the meantime. Am I missing something? I'm getting:

Unable to find a config file and none was specified.

Code

const npsUtils = require('nps-utils');

// Reference to config file
const configFile = __filename;

compile: {
  description: `Compile Sass and JavaScript for development`,
  script: npsUtils.series.nps(`sass.dev --config ${configFile}`, `js.dev --config ${configFile}`)
}

I'm only able to get things working if I write things out manually:

nps sass.dev --config ${configFile} && nps js.dev --config ${configFile}

Thanks for making! 👏

@RichDonnellan
Copy link

RichDonnellan commented Jun 2, 2017

I dug into the source code and was able to run my compile script after making this change:

series.nps = function seriesNPS(...scriptNames) {
  return series(
    ...scriptNames
    .filter(Boolean)
    .map(scriptName => scriptName.trim())
    .filter(Boolean)
-  .map(scriptName => `nps ${quoteScript(scriptName)}`),
+  .map(scriptName => `nps ${scriptName}`,
  )
}

It's getting confused by me having to redeclare my --config. Perhaps a band-aid fix could be to check for the presence of this flag before setting shouldQuote? I haven't the slightest idea how to proceed other than ditch the method approach.

@kentcdodds
Copy link
Owner

This will actually be mostly a non-issue when this happens (hopefully soon).

@RichDonnellan
Copy link

RichDonnellan commented Jun 2, 2017

Will that issue also address the custom --config being ignored from the package.json? I have it set as so, but still need to address each script included in my package-scripts.js. I'd also think that I shouldn't have to prepend each script with nps since my start already includes it.

Do note that I'm not using nps globally, only as a local dependency.

// package.json
"scripts": {
  "start": "nps --config ./node_modules/package-scripts/package-scripts.js"
}

// package-scripts.js in custom location
const configFile = __filename;

script: sass.dev && js.dev // expected
script: `nps sass.dev --config ${configFile} && nps js.dev --config ${configFile}` // actual

@kentcdodds
Copy link
Owner

It wont, but doing that will make solving this issue easier.

@RichDonnellan
Copy link

Cool; thanks for the quick replies.

Do you want me to open an issue to address this within nps? It's still a problem outside of using nps-utils.

@kentcdodds
Copy link
Owner

This is already tracked here: sezna/nps#139

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

2 participants