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 turning off node_modules default exclude via flag #912

Merged
merged 3 commits into from Apr 2, 2019

Conversation

hershmire
Copy link
Contributor

@hershmire hershmire commented Aug 24, 2018

This PR is part of resolving issue #898. The idea is to be able to turn off the default exclusion of **/node_modules/** for users that need/want to support local node_modules. As mentioned in that issue, the negateExclude (i.e. !**/node_modules/**) doesn't work for cases when you still want your exclusion list to still exclude matches within your local node_modules directory.

Related PRs for solving the above issue:

NOTE: this test-exclude PR (istanbuljs/istanbuljs#213) will need to get merged and published before this is effective.

@bcoe
Copy link
Member

bcoe commented Oct 6, 2018

@leslc @hershmire I'm open to this pull request, and your patches across the various Istanbul libraries are wonderful...

please help me better understand why you're running unit tests inside the node_modules folder, this feels really weird to me ... mainly because tests tend to be something people remove when publishing a module, and I think of code in node_modules as the code of your dependents, not your own code.

tldr; I'd love to better understand what motivated this pattern (partially as a product manager at npm, Inc, more so than a maintainer of nyc).

@hershmire
Copy link
Contributor Author

@bcoe This issue I wrote up back in August might answer some of your questions – https://github.com/istanbuljs/nyc/issues/898.

@hershmire
Copy link
Contributor Author

@bcoe Any update?

@JaKXz
Copy link
Member

JaKXz commented Jan 5, 2019

@hershmire I'm sorry for the late reply & if what I say next might be frustrating to you:

If the node_modules that are in those sub-projects/pages are actually local modules (that you hand rolled in place) and not npm installed into those sub-projects/pages, why does the folder have to be called node_modules? CJS requires can work with relative paths so I'm not sure that adding all this cyclomatic complexity to nyc/istanbul is the right approach. As @coreyfarrell said in #898 there are more complex use cases with mono-repos that are becoming more common now to support, so unless this plays nice I don't see this being implemented TBPH.

to anyone: please correct me if I'm wrong.

@bcoe
Copy link
Member

bcoe commented Jan 19, 2019

@JaKXz @hershmire I would love to add better semantics for including node_modules, but I think we should do this work in the test-exclude module, rather than in nyc itself 👍 let's try to pick this work back up though.

@bcoe bcoe closed this Jan 19, 2019
@coreyfarrell coreyfarrell reopened this Apr 2, 2019
It's possible for `config.excludeNodeModules` to be undefined, this is viewed by testExclude as 'not true'.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.349% when pulling 122e9d9 on hershmire:exclude-node_modules-flag into 3eb0e37 on istanbuljs:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.349% when pulling 122e9d9 on hershmire:exclude-node_modules-flag into 3eb0e37 on istanbuljs:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.349% when pulling 122e9d9 on hershmire:exclude-node_modules-flag into 3eb0e37 on istanbuljs:master.

@coreyfarrell
Copy link
Member

@hershmire Sorry for the long delay on this, my intent is that this feature will be available in nyc 14.

@bcoe this feature is already added to test-exclude via istanbuljs/istanbuljs#213, now we just need to expose it to users via nyc.

@AndrewFinlay we will want to add the --exclude-node-modules option to nyc instrument. I didn't add that to this change because I don't want to cause conflicts with #1007 which is nearly ready for merge.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM with @coreyfarrell's approval.

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

Successfully merging this pull request may close these issues.

None yet

5 participants