build: Switch from npmignore to files field #9991
Conversation
packages/astro/package.json
Outdated
| "cjs", | ||
| "esm", | ||
| "types", | ||
| "types-ts3.8", |
There was a problem hiding this comment.
Astro doesn't have integration and types-ts3.8 on npm. Also I think we should also add README.md files. https://www.npmjs.com/package/@sentry/astro?activeTab=code
There was a problem hiding this comment.
Oh you're right. I believe we had it at some point, so we probably forgot to remove it previously.
There was a problem hiding this comment.
We don't need to explicitly add READMEs. They are included by npm by default just like the license and the package json itself: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#files
| @@ -17,6 +17,12 @@ | |||
| "node": ">=18.14.1" | |||
| }, | |||
| "type": "module", | |||
| "files": [ | |||
| "cjs", | |||
There was a problem hiding this comment.
m (not just relevant for this file but for all):
Right now, the files array's contents point to paths relative to <package>/build. This conflicts a bit with the entry points semantics where we point to files from <package>. There's good reason for both but we could consider unifying this to <package> to avoid confusion.
WDYT? I don't have a strong opinion that we should do this because I'm well aware of this fact but maybe it'd be worth doing so. The downside is that we need to rewrite the array in prepack.ts just like we do for the entry points.
If our plan is still to remove prepacking, feel free to completely disregard this.
scripts/prepack.ts
Outdated
| if (fs.existsSync('.npmignore')) { | ||
| ASSETS.push('.npmignore'); | ||
| } |
There was a problem hiding this comment.
l: I think we already check for existence further below when copying the assets. Do we need this check here?
There was a problem hiding this comment.
Down below we check for an asset, but we throw if it doesn't exist. I could switch the logic below to just copy if a file exists and noop otherwise.
scripts/get-files-in-tarballs.sh
Outdated
| @@ -0,0 +1,46 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Maybe stupid question, but where/why are we using this? 😅
There was a problem hiding this comment.
See the PR description. I was using this to verify that the files in the tarballs were the same before and after my changes. I want to keep this file around because I have a feeling that we are gonna make adjustments to the prepack scripts soon.
There was a problem hiding this comment.
yeah, that makes sense, but do we need to check this in here? I'd remove this if we don't need this (regularly)?
There was a problem hiding this comment.
I can remove it! This PR will persist.
This whole
.npmignoremess lead to issues in the past and we wanted to change it since forever so I am using this phase of pondering and loneliness to finally fix it.Tarball files before (generated with
sh scripts/get-files-in-tarballs.sh before.txt): https://gist.github.com/lforst/c74044bcf247125111428d31489b3977