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.

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

Vite: Cleanup and prebundle dependencies #29302

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 8, 2024

Works on #29038

What I did

Before After
pkg-size image image
npmgraph image image

Sources

https://pkg-size.dev/@storybook%[email protected]?no-peers

https://pkg-size.dev/@storybook%[email protected]?no-peers

https://npmgraph.js.org/?q=@storybook/[email protected]#hide=

https://npmgraph.js.org/?q=@storybook/[email protected]#zoom=w

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/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 pull request has been released as version 0.0.0-pr-29302-sha-21f4ff11. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information
Published version 0.0.0-pr-29302-sha-21f4ff11
Triggered by @JReinhold
Repository storybookjs/storybook
Branch jeppe/prebundle-builder-vite
Commit 21f4ff11
Datetime Tue Oct 8 20:23:17 UTC 2024 (1728418997)
Workflow run 11243150011

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=29302

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.7 MB 78.7 MB 0 B 0.83 0%
initSize 148 MB 148 MB -21.1 kB -3.09 0%
diffSize 69.3 MB 69.3 MB -21.1 kB -2.87 0%
buildSize 6.79 MB 6.79 MB 0 B 4.22 0%
buildSbAddonsSize 1.5 MB 1.5 MB 0 B - 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.83 MB 1.83 MB 0 B 0.58 0%
buildSbPreviewSize 270 kB 270 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.8 MB 3.8 MB 0 B 0.58 0%
buildPreviewSize 2.99 MB 2.99 MB 0 B 4.36 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 16.5s 13.9s -2s -612ms 0.05 -18.7%
generateTime 20.6s 23.5s 2.9s 0.83 12.3%
initTime 13.5s 16.2s 2.6s 0.64 16.5%
buildTime 8.2s 9.3s 1s -0.36 11.3%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6.7s 6.2s -525ms -0.89 -8.5%
devManagerResponsive 4.1s 4.1s -33ms -0.81 -0.8%
devManagerHeaderVisible 587ms 508ms -79ms -1.18 -15.6%
devManagerIndexVisible 618ms 536ms -82ms -1.17 -15.3%
devStoryVisibleUncached 1s 972ms -55ms -0.59 -5.7%
devStoryVisible 617ms 535ms -82ms -1.18 -15.3%
devAutodocsVisible 495ms 455ms -40ms -0.87 -8.8%
devMDXVisible 519ms 427ms -92ms -1.22 -21.5%
buildManagerHeaderVisible 492ms 459ms -33ms -1.04 -7.2%
buildManagerIndexVisible 495ms 460ms -35ms -1.15 -7.6%
buildStoryVisible 525ms 527ms 2ms -0.81 0.4%
buildAutodocsVisible 445ms 426ms -19ms -1.06 -4.5%
buildMDXVisible 496ms 455ms -41ms -0.64 -9%

Greptile Summary

This PR optimizes the @storybook/builder-vite package by simplifying dependencies and removing specific configurations.

  • Removed several dependencies and moved others to devDependencies in code/builders/builder-vite/package.json
  • Simplified peerDependencies in package.json, potentially reducing package size
  • Removed Glimmer-specific plugin configuration from code/builders/builder-vite/src/vite-config.ts
  • These changes may impact users of the Glimmer framework with Storybook

Copy link

nx-cloud bot commented Oct 8, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 21f4ff1. 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 target

Sent with 💌 from NxCloud.

@JReinhold JReinhold marked this pull request as ready for review October 8, 2024 21:13
Comment on lines -122 to -127
// TODO: framework doesn't exist, should move into framework when/if built
if (frameworkName === '@storybook/glimmerx-vite') {
const plugin = require('vite-plugin-glimmerx/index.cjs');
plugins.push(plugin.default());
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just cleaned up a bit. @storybook/glimmerx-vite has never existed, this code path can never been triggered, and it doesn't seem like there's been any movement in glimmerx for 3 years.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@JReinhold JReinhold merged commit be183d9 into next Oct 9, 2024
65 of 72 checks passed
@JReinhold JReinhold deleted the jeppe/prebundle-builder-vite branch October 9, 2024 13:20
@github-actions github-actions bot mentioned this pull request Oct 9, 2024
5 tasks
@tobiasdiez
Copy link
Contributor

Did you also check these numbers in "real" applications? Almost every vite / rollup / esbuild plugin uses magic-string if it performs code source manipulation. Other popular usage relevant for storybook are vue and vitest. Thus there is a very high chance that once you use vite, you already have magic-string in your dependency tree. Thus, if you bundle it, then the actual install size increases. Based on the install numbers, a similar argument can probably be made for find-cache-dir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants