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

Nyc should prefer .nycrc config over the package.json nyc stanza #891

Closed
AndrewFinlay opened this issue Jul 17, 2018 · 11 comments
Closed

Nyc should prefer .nycrc config over the package.json nyc stanza #891

AndrewFinlay opened this issue Jul 17, 2018 · 11 comments

Comments

@AndrewFinlay
Copy link
Contributor

Expected Behavior

Nyc should prefer config found in an explicitly specified .nycrc file to config specified in the package.json nyc stanza. This allows you to override existing config using the cli.

Observed Behavior

Nyc prefers config from the nyc stanza in package.json

Bonus Points! Code (or Repository) that Reproduces Issue

Run nyc from the root of this repo with a .nycrc file specified via --nycrc-path, the set of exclude files is taken from package.json rather than .nycrc.

Forensic Information

Operating System: macOS High Sierra
Environment Information:
node v8.11.1
npm v6.1.0

This looks like it's an order of operations issue in "config-util.js", where .pkgConf('nyc', cwd) is applied to the yargs object before we load config from file and apply it to the argv object.

@coreyfarrell
Copy link
Member

I'm leaning towards you being correct, but I'm not sure how to solve this. In any case this will be a breaking change and v13 is already frozen. This will give us time to think about edge cases related to this.

I haven't tried to reproduce your issue but one question: when nyc takes excludes from package.json rather than .nycrc, do both configs contain an exclude option?

@AndrewFinlay
Copy link
Contributor Author

AndrewFinlay commented Jul 31, 2018

I found this one when making changes to nyc to fix the issues I was having with excluding files during instrumentation, see #890 and #893. I'd cloned nyc onto my local machine and was running from the nyc root directory, passing in my .nycrc file location as an option.

That .nycrc file had an exclude section, but it was never used, it picked up the exclude section from nyc's package.json instead, leaving me a little puzzled as to why none of my changes had any effect. So yes, both nyc's package.json nyc stanza and my .nycrc file had an exclude section.

I did take a shot at fixing this within nyc, and it's possible but it will need a bit of refinement, and to be honest I think the issue may really be in the way yargs handles multiple option sets.

Edit: Related to #388

@stale stale bot added the wontfix label Jan 5, 2019
@AndrewFinlay
Copy link
Contributor Author

Bump, stale bot

@JaKXz JaKXz removed the wontfix label Jan 10, 2019
@istanbuljs istanbuljs deleted a comment from stale bot Feb 19, 2019
@stale stale bot added the stale label Apr 20, 2019
@AndrewFinlay
Copy link
Contributor Author

bump

@stale stale bot removed the stale label Apr 21, 2019
@istanbuljs istanbuljs deleted a comment from stale bot Apr 22, 2019
@JaKXz
Copy link
Member

JaKXz commented Apr 22, 2019

@AndrewFinlay I've just pinned this issue for stale bot. Thanks!

@coreyfarrell
Copy link
Member

@AndrewFinlay I suspect you might be correct about this potentially being a yargs issue. Any chance you have time to try putting together an isolation and reporting to the yargs repo?

@AndrewFinlay
Copy link
Contributor Author

Sure thing, but it's not priority #1 at the moment so it may be a week or so.

@coreyfarrell
Copy link
Member

I suspect this will be fixed when we start using @istanbuljs/load-nyc-config to load configuration. In this setup we'll stop using yargs.pkgConf and just provide a load config callback.

@AndrewFinlay
Copy link
Contributor Author

Thought I'd check over this one again and it all seems to work properly now.

I've put together a demo repo here with tap snapshot test that might be useful in nyc.

@coreyfarrell
Copy link
Member

I think we might have testing for this covered in @istanbuljs/load-nyc-config, if not that's probably where it belongs.

@AndrewFinlay
Copy link
Contributor Author

Ok, I'll take a look over there and submit a PR if necessary.
BTW, thanks for your work on this through v15, it's nice to be able to work on nyc without the little roadblocks.

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

No branches or pull requests

3 participants