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

Don't automatically exclude local node_modules #10

Closed
localnerve opened this issue Aug 1, 2016 · 10 comments
Closed

Don't automatically exclude local node_modules #10

localnerve opened this issue Aug 1, 2016 · 10 comments

Comments

@localnerve
Copy link

localnerve commented Aug 1, 2016

It seems this project will not allow any node_modules, not even ones local to a project. Given how node_modules works, why disallow any coverage of code there?

I can help. Please let me know if there is any positive interest in this.

@bcoe
Copy link
Member

bcoe commented Aug 4, 2016

@localnerve I think this update will do the trick:

#11

It will take a little while for it to land in nyc, because we'll need to be fairly careful when pulling it in.

@bcoe
Copy link
Member

bcoe commented Aug 12, 2016

@localnerve mind giving the next release candidate of nyc a spin? You can now provide an empty array to exclude, which will allow you to cover node_modules:

npm cache clear; npm i nyc@next

@localnerve
Copy link
Author

localnerve commented Aug 14, 2016

@bcoe I did this per your instructions, but still did not get local node_modules to show up in coverage. I've tried a few variations of include/exclude, used the nyc 8.0.0 release with babel-plugin-istanbul 1.1.0.
It looks to me like babel-plugin-istanbul (the version I get from npm) still comes with the old test-exclude, so I'm not getting the full/actual effect.
Could you please update the babel-plugin-istanbul dependency for test-exclude to 2.x, bump the pkg version for babel-plugin-istanbul and publish?

@JaKXz
Copy link
Member

JaKXz commented Aug 14, 2016

@localnerve mind if I ask about the motivation behind getting coverage on node_modules? My only issue with it is that everyone with a custom exclude section in their nyc config will have to add node_modules since that was a reasonable default before.

What information does it give you about your codebase/project?

@JaKXz
Copy link
Member

JaKXz commented Aug 14, 2016

Also I'd say this has been fixed in v2.1.x

@bcoe
Copy link
Member

bcoe commented Aug 14, 2016

@JaKXz I felt that it would be interesting to be able to see how covered your dependencies in node_modules are. Due to node_modules always being added to the exclude list, answering this question was unfortunately impossible -- I felt that this breaking change was worthwhile, as it gives folks more flexibility.

@JaKXz
Copy link
Member

JaKXz commented Aug 14, 2016

@bcoe yeah I agree about the flexibility :) I'm just curious about how it's empowering to know "how covered your dependencies are" apart from their hopefully documented coverage... I guess I just need to see an example of it in action

@localnerve
Copy link
Author

@JaKXz No problem! I'm always open to sharing and receiving feedback, it's how I learn.

Motivations

Here is a project that uses local node_modules, as a basis. It uses them at the project level, service-worker level, and for tests.
Using node_modules as outlined here, allows me to keep simple local modules with the code it is meant for (shared code for what I'm working on that may not be suitable for public npm or their own repos).

Just "go"

To make every shared module a universally reusable thing adds time/$$ that may or may not make sense. Even to make that conclusive decision takes time. With local node_modules, you can just code and "go" forward. As the project unfolds, it will become more clear where that module should live and evolve. That doesn't have to be solved right now. In development, just "go" and solve the project specific problems - Make it a local node_module, at least for now.

Cleaner

Eliminates '../../..' import specifications from code altogether.

Simple, Self Documenting

At a glance, you can see what the shared modules for a section of code are. Once you see node_modules you know the modules in there are shared for modules in this sub-tree.

Aggregate Shared

Once you start using local node_modules, you have an aggregate of shared modules that are available for module reference at sub-trees with zero configuration ("just use Node"). Simple folder organization is how you configure it and prevent collisions.

Drawbacks
  1. You have to maintain a postinstall script for project level modules. I've found this not a big deal in my case
  2. Some OSS tools assume to exclude all node_modules, not just the root node_modules (/node_modules), which IS for installed "vendor" code - not project specific code. Usually there is a work-around via configuration in these projects, but sometimes there are bugs. So far, this has just been the babel-register project and in that case it has a PR marked as "new feature".

@localnerve
Copy link
Author

Resolved with nyc 8.1.0, babel-plugin-istanbul 2.0.0.

@hershmire
Copy link
Contributor

This doesn't seem to be resolved if you are looking to only provide excludes to nyc. We have use-cases similar to @localnerve's where within our applications we use local node_modules (./src/node_modules) that are treated as unpublished local modules, committed in our repository. Of course we have root node_modules – i.e. /node_modules that we would still like to be excluded but given the **/node_modules/** exclusion glob in test-exclude (https://github.com/istanbuljs-archived-repos/test-exclude/blob/master/index.js#L35 & https://github.com/istanbuljs-archived-repos/test-exclude/blob/master/index.js#L106), there is no way to specify an exclusion of the root node_modules files but not exclude our local ./src/node_modules files.

If I apply a negate exclude per the docs ('!**/node_modules/**'), it gets applied after all exclude filtering which doesn't help me specify exclusion of root modules only. Am I missing some details in how to properly handle this or is this not doable with the current implementation?

Feature request would be to only exclude root node_modules by default – ./node_modules/**.

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

4 participants