Vue3: Clear stale args/globals when nextArgs is empty in updateArgs#34409
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved an early-return in the Vue3 renderer's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@huang-julien the |
|
@whdjh thanks for that PR! Looks good to me! If you're up for it, I think it'd be great to write an end-to-end test where you navigate from a story with a forced global (e.g. background) to a story without any globals, and verify that the forced global is no longer applied. I'm happy to take time to show you how to run those tests locally! Feel free to ping Steve Dodier-Lazaro on our Discord server. Also, FYI, we have an office hour available for contributors at 3PM CET every Monday! |
|
@Sidnioulz Thanks for the kind words and the great suggestion! 🙏 I haven't worked with e2e tests before, so I'd love to take you up on the offer to learn how to run them locally! I'll hop on the Discord server and reach out to Steve. I can't get to it right away, but I'm planning to pick it up on Thursday (April 2nd). Appreciate the support! |
|
@Sidnioulz
|
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 76 KB | 76 KB | 🎉 -48 B 🎉 |
| Dependency size | 32.23 MB | 32.25 MB | 🚨 +19 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 20.52 MB | 20.47 MB | 🎉 -41 KB 🎉 |
| Dependency size | 16.56 MB | 16.56 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 140 KB | 140 KB | 🚨 +54 B 🚨 |
| Dependency size | 30.44 MB | 30.46 MB | 🚨 +19 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 189 | 189 | 0 |
| Self size | 15 KB | 15 KB | 0 B |
| Dependency size | 28.94 MB | 28.96 MB | 🚨 +19 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 534 | 0 |
| Self size | 650 KB | 650 KB | 🎉 -120 B 🎉 |
| Dependency size | 60.21 MB | 60.23 MB | 🚨 +18 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 92 | 92 | 0 |
| Self size | 1.12 MB | 1.12 MB | 0 B |
| Dependency size | 23.28 MB | 23.29 MB | 🚨 +16 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 121 | 121 | 0 |
| Self size | 30 KB | 30 KB | 0 B |
| Dependency size | 24.35 MB | 24.36 MB | 🚨 +16 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 82 | 82 | 0 |
| Self size | 36 KB | 35 KB | 🎉 -148 B 🎉 |
| Dependency size | 21.06 MB | 21.07 MB | 🚨 +16 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 271 | 271 | 0 |
| Self size | 23 KB | 23 KB | 🎉 -12 B 🎉 |
| Dependency size | 44.82 MB | 44.84 MB | 🚨 +18 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 16 KB | 16 KB | 0 B |
| Dependency size | 33.49 MB | 33.51 MB | 🚨 +19 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 782 KB | 782 KB | 0 B |
| Dependency size | 68.16 MB | 68.14 MB | 🎉 -22 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 177 | 177 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 66.68 MB | 66.66 MB | 🎉 -22 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 1.04 MB | 1.04 MB | 🎉 -363 B 🎉 |
| Dependency size | 37.07 MB | 37.03 MB | 🎉 -41 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 167 | 167 | 0 |
| Self size | 18 KB | 18 KB | 🎉 -122 B 🎉 |
| Dependency size | 31.44 MB | 31.46 MB | 🚨 +19 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 58 | 58 | 0 |
| Self size | 1.44 MB | 1.44 MB | 🎉 -564 B 🎉 |
| Dependency size | 13.22 MB | 13.23 MB | 🚨 +19 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
huang-julien
left a comment
There was a problem hiding this comment.
Great work and thank you for your PR ! 👍
Closes #34319
What I did
Removed the early return in
updateArgs()that skipped processing whennextArgswas an empty object ({}).When
updateArgs()was called withnextArgs = {}, the early return on line 156 prevented the deletion loop from running. This meant that resetting args (or clearing toolbar globals back to{}) had no effect on the Vue reactive objects — stale keys remained alive in the decorator.By removing the early return, the deletion loop now always runs. When
nextArgsis{}, every key incurrentArgsis deleted, resulting in a properly empty reactive object.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarn task --task sandbox --start-from=install --template vue3-vite/default-tsargTypesdefined but no defaultargsDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH 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.tsMake 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/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Before
Mov
before.mov
After
Mov
after.mov
Summary by CodeRabbit
Bug Fixes
Tests