-
Notifications
You must be signed in to change notification settings - Fork 889
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
Remove (package.json).files #1673
Conversation
@@ -6,19 +6,6 @@ | |||
"type": "commonjs", | |||
"types": "pino.d.ts", | |||
"browser": "./browser.js", | |||
"files": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid removing this much would result in test
folder still be published.
Instead, we should remove the single line from this file - the "test" array element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid removing this much would result in
test
folder still be published.
Yes. That is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any explanation why people want /test/
files uploaded to npm (and then to millions of computers world wide) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that they can be referenced when navigating a project's source tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After git checkout
anyone can navigate the source tree.
Why do we upload the "test" fodler to NPM?
https://www.npmjs.com/package/pino?activeTab=code
I don't see any benefits in this change. |
@kibertoad would you see benefits in removing the "test" folder (only!) from the npm tarball? |
@koresar I do, but I think I'm in the minority in the pino team. There was this discussion before, and broadly pino (and fastify) teams consider tests to be part of project documentation, and prefer it bundled for easy perusal by consumers of the libraries. |
Oh my god. Degrading the DX of millions to improve the DX of a few. How about giving a link in Readme:
|
First, how are these a degradation of your experience? Second, a link in a readme isn't going to work when an internet connection is not available or is very unreliable. |
Not a fan of unnecessary files being added either from both an environmental and security perspective, but understand it makes the packages easier to maintain by not having to dance with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This addresses what was left unfinished from #1588 and hopefully makes it clear we want the files in the package (re: #1672).