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

include only necessary files in npm package #260

Closed
wants to merge 1 commit into from
Closed

include only necessary files in npm package #260

wants to merge 1 commit into from

Conversation

deepsweet
Copy link

$ tree node_modules/tape -I node_modules

node_modules/tape
├── bin/
│   └── tape*
├── example/
│   ├── static/
│   │   ├── build.sh*
│   │   ├── index.html
│   │   └── server.js
│   ├── stream/
│   │   ├── test/
│   │   │   ├── x.js
│   │   │   └── y.js
│   │   ├── object.js
│   │   └── tap.js
│   ├── array.js
│   ├── fail.js
│   ├── nested.js
│   ├── nested_fail.js
│   ├── not_enough.js
│   ├── throw.js
│   ├── timing.js
│   ├── too_many.js
│   └── two.js
├── lib/
│   ├── default_stream.js
│   ├── results.js
│   └── test.js
├── test/
│   ├── browser/
│   │   └── asserts.js
│   ├── double_end/
│   │   └── double.js
│   ├── exit/
│   │   ├── fail.js
│   │   ├── ok.js
│   │   ├── second.js
│   │   └── too_few.js
│   ├── max_listeners/
│   │   └── source.js
│   ├── require/
│   │   ├── a.js
│   │   ├── b.js
│   │   ├── test-a.js
│   │   └── test-b.js
│   ├── add-subtest-async.js
│   ├── array.js
│   ├── bound.js
│   ├── child_ordering.js
│   ├── circular-things.js
│   ├── deep-equal-failure.js
│   ├── deep.js
│   ├── double_end.js
│   ├── end-as-callback.js
│   ├── exit.js
│   ├── exposed-harness.js
│   ├── fail.js
│   ├── many.js
│   ├── max_listeners.js
│   ├── nested-async-plan-noend.js
│   ├── nested-sync-noplan-noend.js
│   ├── nested.js
│   ├── nested2.js
│   ├── no_callback.js
│   ├── onFinish.js
│   ├── only-twice.js
│   ├── only.js
│   ├── only2.js
│   ├── only3.js
│   ├── order.js
│   ├── plan_optional.js
│   ├── require.js
│   ├── skip.js
│   ├── stackTrace.js
│   ├── subcount.js
│   ├── subtest_and_async.js
│   ├── subtest_plan.js
│   ├── throws.js
│   ├── timeout.js
│   ├── timeoutAfter.js
│   ├── too_many.js
│   └── undef.js
├── .npmignore
├── .travis.yml
├── LICENSE
├── index.js
├── package.json
└── readme.markdown

See NPM docs for details.

@ljharb
Copy link
Owner

ljharb commented Feb 16, 2016

All files are necessary, including tests. npm explore tape 'npm install && npm test' should work.

@ljharb ljharb closed this Feb 16, 2016
@deepsweet
Copy link
Author

example/ and .travis.yml are not necessary.

what if you will have 100 MB fixtures in test/? it shouldn't be included in published package.

@ljharb
Copy link
Owner

ljharb commented Feb 16, 2016

I agree that fixtures perhaps don't need to be included - but few packages have fixtures at all, let alone large ones.

I agree that "example" and the travis config aren't necessary - but to exclude them, we'd have to add .npmignore, which means all the .gitignore entries have to be duplicated - that's nonzero extra complexity, just to save a few hundred K on disk.

@deepsweet
Copy link
Author

there is no need to add .npmignore, see details of my commit.

@deepsweet
Copy link
Author

also there are much more fixtures than people who ever knows about npm explore and especially npm explore package 'npm install && npm test' – why do I want to do this? we have dependencies and devDependencies here for a reason, so I'll clone the repo, install deps and run tests if I want to contribute.

@mistadikay
Copy link

I agree with @deepsweet. Why would someone need to run npm test for the package in their node_modules folder? What is the use case?

Loading unnecessary files is one of the reasons npm installs take so darn long. Anton Rudeshko very well put it in his article:

Speaking in terms of object-oriented design, your npm package should be highly cohesive and loosely coupled. Your package should do only one thing and do it well.

Corey Butler also wrote a nice article about why it's important to minimize module footprints, including test suite:

In the rare situation a developer actually wants to run your test suite on their own local computer, they’ll likely clone or fork it.

@deepsweet
Copy link
Author

$ du -sh node_modules/less/test/

2.9M    node_modules/less/test/

@ljharb
Copy link
Owner

ljharb commented Feb 17, 2016

@deepsweet if we wanted everything included by default, and only certain files excluded, we would need .npmignore. A whitelist like you have here is insanely dangerous - it's very easy to add a new file to your package but forget to add it to the whitelist.

@mistadikay My use case is, when I want to run tests for a module i've installed. I do this occasionally, and as part of automated tests for some of my modules.

Package size is npm's problem to solve - package authors should not be trying to solve it.

@deepsweet
Copy link
Author

ok, I'm going to say goodbye to this TAP implementation because it looks like you are missing the whole point of NPM registry as a package delivery system.

@ljharb
Copy link
Owner

ljharb commented Feb 17, 2016

The package includes its tests. That is what I want delivered.

@mistadikay
Copy link

a whitelist like you have here is insanely dangerous

I beg to differ. Blacklists might be more dangerous since you can forget to exclude something and introduce a potential security threat if there is a sensitive information. If you forgot to include something into whitelist, at worst you'll have to release a patch version.

My use case is, when I want to run tests for a module i've installed

I still don't quite understand why would you test someone else's packages. Isn't it what package maintainers should do? What if they didn't write any tests?

Package size is npm's problem to solve - package authors should not be trying to solve it.

How npm can solve this problem? By making some precarious assumptions on what should be loaded and what should not?

@yoshuawuyts
Copy link

Let's stay kind to the maintainers here - they see value in tests/ and examples/ being sent, saying that they miss the point of npm... is not particularly nice.

Also a gentle reminder that it's the maintainer's call of what to do with this package - if they give you a clear "no" please respect that. Thanks!

@deepsweet
Copy link
Author

It's not nice to chase my PRs even in not related to this maintainer projects. There are still no strong arguments to include tests.

@ljharb
Copy link
Owner

ljharb commented Feb 17, 2016

The strong argument is "I want to be able to run tests in my dependencies when possible". You're creating a huge amount of FUD and future functionality risk by submitting tons of PRs all over the place that use a whitelist approach - files are only downloaded once per version, and are cached. This just isn't a big problem - install time isn't where speed matters.

@deepsweet
Copy link
Author

that's why I said that you are missing something very important. npm
install time IS the big problem since npm@3 and especially for the last
few months, that's why projects like https://github.com/rstacruz/pnpm are
here now, and so on.

I already answered you why blacklist can be more dangerous than whitelist.

sorry but "I want to be able to run tests in my dependencies" is absolutely
uncommon bad pattern.

your point to include tests (even with 100 MB fixtures, because w/o it
tests will be broken) to npm package is totally wrong for me.

@ljharb
Copy link
Owner

ljharb commented Feb 17, 2016

And look, projects like pnpm are solving it and package authors don't have to.

It is better to release a working package with sensitive data than a broken package.

It's fine if it's wrong for you - you're welcome to exclude test files from your own packages. That doesn't mean it's wrong.

@yoshuawuyts
Copy link

@deepsweet don't throw out a strawman, test + examples in this package results in 220kb with no gzip - that's two orders of magnitude off from your statement of 100mb.

There's probably more important things to worry about than <200kb in overhead for a package that is already cached. Visiting pretty much ANY website on the internet will download more assets than that. I agree with @ljharb, file size optimization should be solved by package managers - not by package authors.

@ghost
Copy link

ghost commented Mar 24, 2016

$ curl -s https://registry.npmjs.org/tape/-/tape-`npm view tape version`.tgz | wc -c
19957

20 kilobytes.

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

Successfully merging this pull request may close these issues.

None yet

4 participants