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

Remove (package.json).files #1673

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Remove (package.json).files #1673

merged 1 commit into from
Mar 17, 2023

Conversation

jsumners
Copy link
Member

This addresses what was left unfinished from #1588 and hopefully makes it clear we want the files in the package (re: #1672).

@jsumners jsumners requested a review from mcollina March 17, 2023 13:00
@@ -6,19 +6,6 @@
"type": "commonjs",
"types": "pino.d.ts",
"browser": "./browser.js",
"files": [
Copy link

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.

Copy link
Member Author

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.

Copy link

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) ?

Copy link
Member Author

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.

Copy link

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

@kibertoad
Copy link
Contributor

I don't see any benefits in this change.

@koresar
Copy link

koresar commented Mar 17, 2023

@kibertoad would you see benefits in removing the "test" folder (only!) from the npm tarball?

@kibertoad
Copy link
Contributor

@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.

@koresar
Copy link

koresar commented Mar 17, 2023

Oh my god.

Degrading the DX of millions to improve the DX of a few.

How about giving a link in Readme:

Here are our unit tests, find examples there: <link>

@jsumners
Copy link
Member Author

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.

@Fdawgs
Copy link
Member

Fdawgs commented Mar 17, 2023

@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.

See fastify/skeleton#42

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 .npmignore and package.json files

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@jsumners jsumners merged commit 2d5c522 into master Mar 17, 2023
@jsumners jsumners deleted the packaging branch March 17, 2023 20:40
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants