fix(npm): include the proper files in the npm tarball #594

favna · 2024-04-06T18:48:45Z

Description

fixes #591, I think I broke this in #528 because yarn npm publish works differently with which files it includes in the tarball from npm publish (which just includes everything from the cwd)

I also added a cp of the CHANGELOG.md file, because Yarn likes to include that automatically, just like the README. They used to also show it on the package pages before the redesign (ex: https://v3.yarnpkg.com/package/@sapphire/framework) so I can only assume it will be added back eventually.

Motivation and Context

Emergency fix, as the current release on npm is broken.

How Has This Been Tested?

I ran the cd commands locally:

yarn install
yarn build
cp ../../README.md .
cp ../../CHANGELOG.md .
yarn pack --dry-run

The only differing factor here being the last command because obviously I do not want to actually publish but this does show what gets put in the tarball:

yarn pack --dry-run    
➤ YN0000: CHANGELOG.md
➤ YN0000: README.md
➤ YN0000: lib/cjs/index.cjs
➤ YN0000: lib/cjs/index.cjs.map
➤ YN0000: lib/cjs/index.d.cts
➤ YN0000: lib/cli/cli.js
➤ YN0000: lib/esm/index.d.ts
➤ YN0000: lib/esm/index.js
➤ YN0000: lib/esm/index.js.map
➤ YN0000: package.json
➤ YN0000: Done in 0s 27ms

Screenshots / Logs (if applicable)

Types of Changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (no code change)
Refactor (refactoring production code)
Other: releases

Checklist:

My code follows the code style of this project.
I have updated the documentation accordingly.
I have formatted the code with rustfmt.
I checked the lints with clippy.
I have added tests to cover my changes.
All new and existing tests passed.

fixes orhun#591

codecov-commenter · 2024-04-06T18:53:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.27%. Comparing base (9864780) to head (a21d221).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #594   +/-   ##
=======================================
  Coverage   41.27%   41.27%           
=======================================
  Files          15       15           
  Lines        1071     1071           
=======================================
  Hits          442      442           
  Misses        629      629

Flag	Coverage Δ
unit-tests	`41.27% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

favna · 2024-04-06T18:57:16Z

npm/git-cliff/package.json

To clarify, what this essentially says is "include only the files in lib, and include everything in there". This ensures that the JavaScript files are also included. Yarn (and npm too for that matter) automatically include the package.json and README.md when found, so need to specify them.

aminya · 2024-04-06T19:20:42Z

npm/git-cliff/package.json

@@ -22,6 +22,9 @@
      }
    }
  },
+  "files": [
+    "lib"


Could you include the source Typescript files as well?

There is absolutely no reason to do so. Types are provided through the output .d.ts files and node, bun and deno all will never even load the source typescript files. In fact, Node cannot even load the source typescript files.

Also due to the export mapping (a few lines above this) the files in src won't even be accessible. Node will just say they're not exported. The only way to read them would with a manual readFile call.

Well, my build system Parcel is capable of using the source files directly. I also prefer untransformed source code that is on GitHub. Given what has been going on with the xz distribution, you should consider this.

I don't see any relationship to what happened to xz. It's also not "going on", it's already resolved. Please show me the documentation where parcel says they use the source files. I've used parcel before and it doesn't. Please also explain what your exact use case is that you use parcel to not only consume the git-cliff npm package but also parcel bundle it. Parcel is for bundling webapps after all.

I'm not well versed in these topics so I cannot comment. But I would appreciate a ELI5 summary 😊

Sure!

@aminya wants me to include the TypeScript files from src in the tarball. This is something that no library ever does. Search the favourite big-name packages written in TypeScript on https://yarnpkg.com as they have a file viewer for what's in the tarball, and you'll see that none of them include the source TypeScript files. This is because runtimes and bundlers will never access them. This is despite what @aminya says about Parcel, as described by my previous comment. Even if Parcel would access those files, it would be the only bundler to do so and it's far from the most popular bundler (npmtrends). Ultimately, including the source TypeScript files is unnecessary bloat and increasing filesize for no actual gain.

Optimizing the package size seems unnecessary, and the fact that the popular packages do not provide source files just reveals the wide attack surface in open source.

The question remains though. How can I make sure the JavaScript files in the dist are exactly derived from the source TypeScript files in the repository? Do you have a byte-for-byte reproducible build and release process?

Optimizing the package size seems unnecessary.

This is indeed crucial as it has a direct impact on the download size, and it's important to note that not all users have access to high-speed internet. Moreover, since this also includes a programmatic API, it is not solely installed as a global dependency and it would thus also affect other packages that implement git-cliff, my own favware/cliff-jumper among them. This package is the main reason why I even started contributing to git-cliff in the first place. Furthermore, Yarn Berry (a.k.a. Yarn version 2 and higher) has ceased to support global dependencies.

How can I make sure the JavaScript files in the dist are exactly derived from the source TypeScript files in the repository? Do you have a byte-for-byte reproducible build and release process?

I understand your concerns. Indeed, you are technically correct. However, it is important to trust the open source community. The recent exploit in one library does not necessarily warrant an Nth degree heightened level of scrutiny. If the issue and my subsequent PR had occurred a few weeks ago, prior to the xz incident, it likely would not have attracted this level of attention.

It is not practical to compare the source TS files with the compiled JS files in your node_modules folder for every package. If we were to do this for every package, the process would be extremely time-consuming. Have you considered the number of files in your node_modules folder or the number of transitive dependencies you’re pulling in? This package alone installs a total of 18 packages that would all require analysis.

It is crucial to maintain a balanced perspective and not overreact to individual incidents. We should continue to support and have faith in the open source community.

To further illustrate the complexity of your proposal, I have attached the tarball that would be published after this PR. Please note that it is zipped because GitHub does not support the upload of .tgz files. You can find it here: git-cliff.zip

Additionally, consider another repository that includes a significant higher amount of code, another project I maintain: sapphiredev/framework. The package can be found here: package.zip

Thanks for the detailed summary and your thoughts! I agree that we should still have faith in the open source community. I don't have a strong preference between including/not including the TS files due to the fact that it's an whole another ecosystem that I'm not a part of. I can still very clearly see @aminya's concerns, but I'd say we can discuss this elsewhere for the sake of not blocking this PR.

I'm happy to include the TS files if there are strong arguments with practical and proven reproducibility along with a PR.

orhun

Hey guys! Thanks for your comments/work on this. I'm curious if there is any way to test the programmatic API via CI to ensure that we don't hit this issue again and everything works correctly.

npm/git-cliff/yarn.lock

favna · 2024-04-07T22:21:03Z

Hm we could maybe check the files returned by yarn pack --dry-run?

Edit: added in 9037521

…ping versions to update the lockfile

orhun

Thanks!

fix: set proper files to include in the npm tarball

137097b

fixes orhun#591

favna requested a review from orhun as a code owner April 6, 2024 18:48

favna commented Apr 6, 2024

View reviewed changes

aminya reviewed Apr 6, 2024

View reviewed changes

orhun reviewed Apr 7, 2024

View reviewed changes

npm/git-cliff/yarn.lock Show resolved Hide resolved

favna added 2 commits April 8, 2024 09:20

ci: add job that checks npm tarball content

9037521

fix(release): update the release script to run yarn install after bum…

a21d221

…ping versions to update the lockfile

orhun changed the title ~~fix: set proper files to include in the npm tarball~~ fix(npm): include the proper files in the npm tarball Apr 9, 2024

orhun approved these changes Apr 9, 2024

View reviewed changes

orhun merged commit 800c896 into orhun:main Apr 9, 2024
45 checks passed

favna deleted the fix/npm-tarball-included-files branch April 12, 2024 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(npm): include the proper files in the npm tarball #594

fix(npm): include the proper files in the npm tarball #594

favna commented Apr 6, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024 •

edited

Loading

favna Apr 6, 2024

aminya Apr 6, 2024

favna Apr 6, 2024 •

edited

Loading

favna Apr 6, 2024 •

edited

Loading

aminya Apr 6, 2024

favna Apr 6, 2024

orhun Apr 7, 2024

favna Apr 8, 2024 •

edited

Loading

aminya Apr 8, 2024

favna Apr 8, 2024 •

edited

Loading

orhun Apr 9, 2024

orhun left a comment

favna commented Apr 7, 2024 •

edited

Loading

orhun left a comment

fix(npm): include the proper files in the npm tarball #594

fix(npm): include the proper files in the npm tarball #594

Conversation

favna commented Apr 6, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots / Logs (if applicable)

Types of Changes

Checklist:

codecov-commenter commented Apr 6, 2024 • edited Loading

Codecov Report

favna Apr 6, 2024

Choose a reason for hiding this comment

aminya Apr 6, 2024

Choose a reason for hiding this comment

favna Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

favna Apr 6, 2024 • edited Loading

Choose a reason for hiding this comment

aminya Apr 6, 2024

Choose a reason for hiding this comment

favna Apr 6, 2024

Choose a reason for hiding this comment

orhun Apr 7, 2024

Choose a reason for hiding this comment

favna Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

aminya Apr 8, 2024

Choose a reason for hiding this comment

favna Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

orhun Apr 9, 2024

Choose a reason for hiding this comment

orhun left a comment

Choose a reason for hiding this comment

favna commented Apr 7, 2024 • edited Loading

orhun left a comment

Choose a reason for hiding this comment

favna commented Apr 6, 2024 •

edited

Loading

codecov-commenter commented Apr 6, 2024 •

edited

Loading

favna Apr 6, 2024 •

edited

Loading

favna Apr 6, 2024 •

edited

Loading

favna Apr 8, 2024 •

edited

Loading

favna Apr 8, 2024 •

edited

Loading

favna commented Apr 7, 2024 •

edited

Loading