-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
ViteBuilder: Allow viteFinal to modify the configuration before getOptimizeDeps triggers resolveConfig #29790
base: next
Are you sure you want to change the base?
ViteBuilder: Allow viteFinal to modify the configuration before getOptimizeDeps triggers resolveConfig #29790
Conversation
2a8c9b8
to
88a9c55
Compare
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.
LGTM
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
88a9c55
to
5f48f08
Compare
…timizeDeps triggers resolveConfig
5f48f08
to
ce9f6b1
Compare
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.
LGTM
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 15a62ef. 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. |
@cr7pt0gr4ph7, thank you for putting together this pull request and helping us improve Storybook. We appreciate it 🙏 ! @IanVS do you mind looking into this as well when you're able? Thanks in advance |
What I did
The
getOptimizeDeps(config, ...)
functions internally usesvite.resolveConfig(config).createResolver()
, which causes problems if some plugin throws an error because the configuration is not valid. This PR defers thegetOptimizeDeps
invocation until afterviteFinal
had a chance to fix up the configuration.The specific issue I encountered was that the Vike framework, which builds upon Vite, does not like it if
base
is set to"./"
, while Storybook forcesbase: "./"
and currently invokesgetOptimizeDeps
before the user has a chance to rectify the issues throughviteFinal
.This change should only be observable to consumers in two cases:
getOptimizedDeps
failed due to a plugin throwing an error during initialization (so it was broken anyway) because of an invalid configuration.viteFinal
is used to remove dependencies that were added bygetOptimizedDeps
. This is not possible anymore.viteFinal
twice, once to produce the preliminary resolved configuration used bygetOptimizedDeps
, and the second time on the unresolved preliminary configuration +optimizeDeps
to produce the final configuration.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
vite-builder
to build Vite applications will implicitly test the affected code as well.Manual testing
yarn task --task dev --start-from=install --template react-vite/default-ts
Documentation
viteFinal
already aligns with the behaviour implemented here.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-storybook/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>
Greptile Summary
Reorders Vite server initialization to ensure framework-specific configuration changes are applied before dependency optimization occurs, preventing plugin initialization errors.
code/builders/builder-vite/src/vite-server.ts
to defergetOptimizeDeps
until afterviteFinal
runsbase
configuration different from Storybook's defaultresolveConfig
is called before configuration is fully preparedviteFinal
removes dependencies💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!