-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Maintenance: Disable minification in build scripts #28595
Conversation
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.
PR Summary
- Disabled
minifyIdentifiers
andminifySyntax
in/code/core/scripts/prep.ts
- Modified
getESBuildOptions
in/scripts/prepare/addon-bundle.ts
to disable minification - Updated
getESBuildOptions
in/scripts/prepare/bundle.ts
to disable minification
3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 77d10d5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
@ndelangen Your opinion is requested here. |
It seems to be increasing the size of build storybook substantially @valentinpalkovic |
I had accidentally also disabled the minification for the manager build. The numbers should be better now. Could you please trigger a new benchmark? |
It auto triggers, the number do not look too great, they are increasing pretty significantly upwards. |
I tried to reproduce this locally by running I would also argue that the distributed size is not super important, depending on the use case. If it's just used as an internal dev tool, then I would gladly have a slightly larger storybook dist then having to uncrypt errors such as
If the storybook is intended for a larger audience and has many views, one can activate code minimization of the bundler. The important point is that the end-user can decide which scenario he prefers. |
@storybookjs/core what do we think? Should we proceed with this, trade some perf for debug-ability? |
Personally I'm very much in favor of this trade-off, and I know @IanVS has requested the same thing before. I don't know about you all, but multiple times a week I dive into a package in I don't understand why the build sizes are different though, I'd expect a built Storybook to maintain it's current size. |
Thanks for the investigation @tobiasdiez. We've discussed this in the team and we don't think the timing of this is great. We're currently putting a lot of effort into making Storybook smaller and faster across the board, including minimising dependencies, ESM-only experiments, etc. Not minifying the package would counter-act that, and we don't think that's the right trade-off right now. I think we should look into this again when we're happy with our size-reduction efforts in a few month, to see how much the difference is by then. |
Happy to hear that this is a priority. Especially, ESM-first sounds like a very good idea.
I honestly don't agree here. Your efforts in using esm treeshaking and minimizing dependencies would still have a huge impact, regardless of minimizing the code or not. If you look in the benchmark above you see that the for the user-code storybook/code/builders/builder-webpack5/src/index.ts Lines 291 to 304 in 6388f93
If it would pass through the normal build pipeline, then it would also be minified before deploying. |
That's fair, and I can somewhat agree with that. But right now the priority is to get the size down, because it has continuously been a top complaint for years, that Storybook is "bloated". There are many ways to interpret and solve that, one of them being the package download size, that we want to focus on first. Making it easier to debug Storybook is also a big win, but it only helps a tiny minority (including me) compared to the countless size complaints.
Yeah if we did end up not shipping minified code, we'd definitely need to minify everything during build, and there's a performance penalty to doing that we'd need to investigate as well. Could be very little though. |
Closes #23970
What I did
Remove minification in build scripts so that the distributed files are human-readable. This makes debugging of storybook issues 1000 times easier. Since the storybook packages are passed through a bundler anyway, minimifaction doesn't provide any real benefit for production.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Run the
scripts/prepare/bundle.ts
script on some packages and look at the output produced indist
.This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
bug
: Internal changes that fixes incorrect behavior.maintenance
: User-facing maintenance tasks.dependencies
: Upgrading (sometimes downgrading) dependencies.build
: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup
: Minor cleanup style change. Will not show up in release changelog.documentation
: Documentation only changes. Will not show up in release changelog.feature request
: Introducing a new feature.BREAKING CHANGE
: Changes that break compatibility in some way with current major version.other
: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/core
team here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>