-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
build: Create internal rollup-utils
package
#9908
Conversation
Only sentry internal eslint config changed. |
packages/rollup-utils/index.mjs
Outdated
@@ -0,0 +1,9 @@ | |||
Error.stackTraceLimit = Infinity; |
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.
why do we need this?
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.
We already had this before. I believe this was so that we get bigger (ie not truncated) stack traces in case of an error.
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.
nice TIL
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.
This significantly affects the build performance.
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 can see that. Will remove. Stack traces during build are somewhat meh anyhow.
scripts/tarball-checksums.sh
Outdated
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.
great idea!
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.
one more thing: Let's please add a small readme to explain the package's purpose
@@ -69,6 +69,7 @@ | |||
"packages/remix", | |||
"packages/replay", | |||
"packages/replay-worker", | |||
"packages/rollup-utils", |
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.
Can we just make this build
? If we move to a different package, we shouldn't change the package name
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 am gonna leave it at rollup-utils
because that is what it is. build
is too generic since building has many more steps than just calling some rollup utils.
packages/rollup-utils/index.mjs
Outdated
@@ -0,0 +1,9 @@ | |||
Error.stackTraceLimit = Infinity; |
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.
This significantly affects the build performance.
size-limit report 📦
|
As preparation for v8 and cleaning up our rollup configs to add
preserveModules: false
without needing to wipe the pain off our memories afterward this PR creates arollup-utils
package.This PR doesn't change any functionality (see comment about checksums below). We can later move this package into a
dev-packages
folder if we so desire.We had to change all of the rollup config files to mjs because of some package boundary stuff. This is hopefully a minor change though. It is also kinda good because rollup requires this in newer versions by default.