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

Update .npmignore #250

Closed
wants to merge 1 commit into from
Closed

Update .npmignore #250

wants to merge 1 commit into from

Conversation

eugenelim
Copy link

Remove test/ folder from being published with the npm package

Remove test/ folder from being published with the npm package
Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is intentionally included; npm explore foo && npm install && npm test should always work.

@ljharb ljharb closed this Feb 15, 2018
@eugenelim
Copy link
Author

Shouldn't npm test work only if you directly git clone the repository? If this is just an npm dependency, we shouldn't pull in the test/ folder

@ljharb
Copy link
Owner

ljharb commented Feb 15, 2018

No, running tests for installed deps is an important debugging tool, and it should work offline.

@mdevils
Copy link

mdevils commented Mar 15, 2021

@ljharb

No, running tests for installed deps is an important debugging tool, and it should work offline.

Important for what use-case?

@ljharb
Copy link
Owner

ljharb commented Mar 15, 2021

Not having an internet connection when i want to run my deps’ tests, to help debug issues.

@mdevils
Copy link

mdevils commented Mar 15, 2021

@ljharb that's what I mean. What is the use-case when you want to run your deps' tests? I never heard about this scenario and almost any npm package comes with tests excluded, that's why I'm asking.

@ljharb
Copy link
Owner

ljharb commented Mar 15, 2021

Every package i maintain includes tests, and i learned the practice back when virtually every package included tests, so I’m not sure how accurate “almost any” is.

The use case is when I’m verifying that the dep is working on my exact platform and node version, with my exact NODE_OPTIONS, on my exact filesystem. It has turned out to be a bug in the dep more often than you likely expect.

@mdevils
Copy link

mdevils commented Mar 15, 2021

@ljharb

so I’m not sure how accurate “almost any” is

if we go through top 100 npm packages, only 7 of them have tests included and 2 (of these 7) are dead / no longer maintained.

Here are the results and a script which produces them: https://gist.github.com/mdevils/0674355bc679e281d57c0cc167b344ce

The use case is when I’m verifying that the dep is working on my exact platform and node version, with my exact NODE_OPTIONS, on my exact filesystem. It has turned out to be a bug in the dep more often than you likely expect.

OK, I see how it might be useful for you. But I believe for everyone else who uses your libs, tests are just space on the hard drive. Especially in case of this package, which has nothing to do with node.js built-in libraries and just operates with strings, arrays and objects.

@ljharb
Copy link
Owner

ljharb commented Mar 15, 2021

"top" is irrelevant; npm has 1.5 million packages, 100 isn't nearly enough of a sample to be significant.

Disk space is effectively infinite and free; we're talking about 70 kilobytes here. The browser cache space taken up by viewing this very page is likely a hundred times that.

@mdevils
Copy link

mdevils commented Mar 15, 2021

@ljharb Whatever's clever 🤷‍♀️

@ljharb ljharb mentioned this pull request Jan 12, 2022
@eugenelim
Copy link
Author

eugenelim commented Feb 7, 2022 via email

@ljharb
Copy link
Owner

ljharb commented Feb 7, 2022

@eugenelim then you should be bundling your application, because otherwise you're always shipping files you don't need, like package.jsons and README.md, but also like implementation files you don't happen to be using.

@ljharb ljharb mentioned this pull request May 6, 2024
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.

3 participants