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.

Sign up for GitHub

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

Jump to bottom

feat(build): Drop prepack step & directly pack /build directory #12656

Merged
merged 14 commits into from
Jun 27, 2024
Merged

feat(build): Drop prepack step & directly pack /build directory #12656

merged 14 commits into from
Jun 27, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Jun 26, 2024 •
edited
Loading

This is mostly an internal change, but it does change the layout of the published package, which shouldn't affect users, unless they depend on non-public things (which nobody should!).

With this change, we will simply pack the packages as-is without moving any folder relativity. This means we do not need a prepack step anymore.

Instead, we define which files should be included in the tarball via files in the package.json.

image

Fixes #12642

There are some tiny fluctuations in the comparison table but as far as I can tell nothing serious/bad.
@mydea mydea self-assigned this Jun 26, 2024
@mydea mydea requested review from a team as code owners June 26, 2024 09:28
@mydea
Copy link
Member Author

mydea commented Jun 26, 2024

cc @nicohrubec we should also do this for the nest package right away!
nicohrubec
nicohrubec reviewed Jun 26, 2024
View reviewed changes
packages/svelte/package.json Outdated
"types",
"types-ts3.8"
"/build",
"LICENCE",
Copy link
Member

Choose a reason for hiding this comment

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

should this be LICENSE maybe?
Copy link
Member Author

Choose a reason for hiding this comment

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

oops, yes it should! 🤦 good catch!
Copy link
Member

Choose a reason for hiding this comment

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

some files including the license are added by default: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#files
Copy link
Member Author

Choose a reason for hiding this comment

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

ah, nice, then I'll just remove this overall
Copy link

Choose a reason for hiding this comment

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

README.* is also included by default.
andreiborza
andreiborza reviewed Jun 26, 2024
View reviewed changes
packages/angular/package.json Outdated
@@ -14,8 +14,8 @@
"/build/fesm2015",
"/build/fesm2020",
"/build/*.d.ts",
Copy link
Member

Choose a reason for hiding this comment

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

What about the *.d.ts.map files?
Copy link
Member Author

Choose a reason for hiding this comment

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

there are none here, actually! But I'll still add this, to be future-safe!
andreiborza
andreiborza approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

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

nice reducing complexity!
AbhiPrasad
AbhiPrasad reviewed Jun 26, 2024
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Can we remove/update https://github.com/getsentry/sentry-javascript/blob/develop/scripts/prepack.ts?
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 26, 2024 •
edited
Loading

size-limit report 📦

Path Size
@sentry/browser 22.22 KB (0%)
@sentry/browser (incl. Tracing) 33.31 KB (0%)
@sentry/browser (incl. Tracing, Replay) 69.09 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 62.42 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 73.15 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 85.75 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 87.61 KB (0%)
@sentry/browser (incl. metrics) 26.5 KB (0%)
@sentry/browser (incl. Feedback) 38.86 KB (0%)
@sentry/browser (incl. sendFeedback) 26.85 KB (0%)
@sentry/browser (incl. FeedbackAsync) 31.45 KB (0%)
@sentry/react 24.97 KB (0%)
@sentry/react (incl. Tracing) 36.36 KB (0%)
@sentry/vue 26.33 KB (0%)
@sentry/vue (incl. Tracing) 35.17 KB (0%)
@sentry/svelte 22.36 KB (0%)
CDN Bundle 23.42 KB (0%)
CDN Bundle (incl. Tracing) 35.05 KB (0%)
CDN Bundle (incl. Tracing, Replay) 69.18 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 74.38 KB (0%)
CDN Bundle - uncompressed 68.8 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 103.66 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed 214.13 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 226.84 KB (0%)
@sentry/nextjs (client) 36.24 KB (0%)
@sentry/sveltekit (client) 33.95 KB (0%)
@sentry/node 132.43 KB (0%)
@sentry/node - without tracing 91.87 KB (+0.01% 🔺)
@sentry/aws-serverless 118.45 KB (+0.01% 🔺)
@mydea
Copy link
Member Author

mydea commented Jun 26, 2024

Can we remove/update https://github.com/getsentry/sentry-javascript/blob/develop/scripts/prepack.ts?

That is removed in this PR! 😅
@mydea
Copy link
Member Author

mydea commented Jun 26, 2024

For reference, in Angular we still pack from ./build, but that is the way to go there - the angular build command does some magic itself for this.
AbhiPrasad
AbhiPrasad approved these changes Jun 26, 2024
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

That is removed in this PR

well that's embarrassing
@mydea mydea merged commit 2a7c22b into develop Jun 27, 2024
140 checks passed
@mydea mydea deleted the fn/no-more-prepack branch June 27, 2024 08:57
@AbhiPrasad AbhiPrasad mentioned this pull request Jun 28, 2024
Merged
AbhiPrasad added a commit that referenced this pull request Jul 2, 2024
@AbhiPrasad
fix(deno): Add prepack for deno build (#12700)
840dd8f 
fixes #12698

Adds back prepack script just for deno which was removed in
#12656 because it's a
breaking change for the deno package which relies on the directory
structure for their import path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants
@mydea @lforst @alexandersorokin @AbhiPrasad @nicohrubec @andreiborza