Vue: Make globals reactive in decorators#33562
Conversation
|
View your CI Pipeline Execution ↗ for commit 75d6e5d
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughMake storyContext.globals reactive and expose per-canvas Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as Storybook Manager
participant Renderer as Vue3 Renderer
participant App as Vue App (per-canvas)
participant Component as Story Component
Manager->>Renderer: renderCanvas(storyContext)
Renderer->>App: create/mount app with reactiveArgs + reactiveGlobals
App->>Component: provide('globals', reactiveGlobals)
Component->>Component: watch reactiveArgs / reactiveGlobals -> re-render
Manager->>Renderer: updateArgs<Args>(nextArgs)
Renderer->>App: mutate reactiveArgs (updateArgs<T>)
App->>Component: reactive update triggers re-render
Manager->>Renderer: updateArgs<Globals>(nextGlobals)
Renderer->>App: mutate reactiveGlobals (updateArgs<T>)
App->>Component: provided globals changed -> dependent watches run -> re-render
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Comment |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 192 | 192 | 0 |
| Self size | 75 KB | 75 KB | 🎉 -6 B 🎉 |
| Dependency size | 32.24 MB | 32.25 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 49 | 49 | 0 |
| Self size | 20.30 MB | 20.32 MB | 🚨 +21 KB 🚨 |
| Dependency size | 16.52 MB | 16.52 MB | 🎉 -4 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 15 KB | 15 KB | 🎉 -6 B 🎉 |
| Dependency size | 28.96 MB | 28.97 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 538 | 538 | 0 |
| Self size | 646 KB | 646 KB | 🎉 -10 B 🎉 |
| Dependency size | 59.22 MB | 59.73 MB | 🚨 +505 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 127 | 127 | 0 |
| Self size | 1.12 MB | 1.12 MB | 🎉 -22 B 🎉 |
| Dependency size | 21.82 MB | 22.32 MB | 🚨 +496 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 159 | 159 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +8 B 🚨 |
| Dependency size | 23.00 MB | 23.61 MB | 🚨 +610 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 117 | 117 | 0 |
| Self size | 35 KB | 35 KB | 🎉 -26 B 🎉 |
| Dependency size | 19.62 MB | 20.11 MB | 🚨 +496 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 278 | 278 | 0 |
| Self size | 24 KB | 24 KB | 🚨 +2 B 🚨 |
| Dependency size | 44.13 MB | 44.64 MB | 🚨 +505 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 16 KB | 16 KB | 🎉 -10 B 🎉 |
| Dependency size | 33.49 MB | 33.50 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 183 | 183 | 0 |
| Self size | 775 KB | 775 KB | 🚨 +219 B 🚨 |
| Dependency size | 67.38 MB | 67.47 MB | 🚨 +92 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 176 | 176 | 0 |
| Self size | 30 KB | 30 KB | 🎉 -4 B 🎉 |
| Dependency size | 65.95 MB | 66.05 MB | 🚨 +92 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 50 | 50 | 0 |
| Self size | 1000 KB | 1000 KB | 🎉 -6 B 🎉 |
| Dependency size | 36.82 MB | 36.84 MB | 🚨 +21 KB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 170 | 170 | 0 |
| Self size | 18 KB | 18 KB | 🚨 +18 B 🚨 |
| Dependency size | 31.26 MB | 31.28 MB | 🚨 +11 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 57 | 57 | 0 |
| Self size | 732 KB | 1.23 MB | 🚨 +494 KB 🚨 |
| Dependency size | 12.94 MB | 12.94 MB | 🚨 +2 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
chakAs3
left a comment
There was a problem hiding this comment.
this looks a straight forward and clean, would be worth to add some test story to showcase this beside the unit tests ?
897c666 to
e6420ef
Compare
I don't think we can call |
|
By the way, something feels off here. I remember this globals update working correctly when I worked on it back in v7. At that time, changing globals from the toolbar and triggering decorators caused a full re-render of the component tree outside of Vue. This looks like a regression introduced by a later commit |
I remember it worked for a brief time around 7.0, and then broke. Reactivity does occur in the decorator function, but the object being returned by the decorator isn't being transformed into a new component. I haven't tried to figure out why/when it broke. The code for args was straightforward enough to copy and I have you to thank for that :) |
e6420ef to
c696db8
Compare
5b33799 to
1aae330
Compare
|
@chakAs3 I've managed to find a code pattern without provide/inject so I've simplified the code a bit. I added Vue framework stories for reactive globals (and reactive updateArgs) alongside E2E tests and documentation changes. |
1aae330 to
12e7f63
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/_snippets/decorator-with-reactive-globals.md`:
- Around line 15-16: The template uses invalid Vue binding syntax
"lang={{globals?.locale || 'en'}}" which will render literally; update the
attribute to use Vue v-bind syntax by replacing the raw mustache binding with a
bound expression (i.e., use :lang with the same expression globals?.locale ||
'en') in both occurrences (the shown snippet around the div with lang and the
other occurrence at lines ~41-42) so the lang attribute is evaluated at runtime
rather than printed as text.
🧹 Nitpick comments (1)
code/e2e-tests/framework-vue3.spec.ts (1)
17-17: Remove stale TODO.The updateArgs test exists below, so this TODO is now misleading.
🧹 Proposed cleanup
- // TODO: add test to prove updateArgs works too
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@code/renderers/vue3/template/stories_vue3-vite-default-ts/decorators.stories.ts`:
- Around line 87-107: The decorator localeDecorator currently declares inject:
['globals'] which is unnecessary and causes Vue dev warnings; remove the inject
property from the returned object in localeDecorator (leaving components, setup,
template intact) so that globals from the decorator context is used directly in
setup (where ctxGreeting is computed using
getCaptionForLocale(globals?.locale)), and verify no other code relies on Vue's
provide/inject for globals.
|
I need to revert this PR because vue-v3--vite--javascript---dev is failing, which was caught by our ci:daily job: Please reopen another PR and set |
Closes #23655
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Unit tests are very low quality. Really, we're testing the underlying update function used to update globals, rather than the whole setup. I'd want a dedicated story instead.
Manual testing
.storybook/preview.tsDocumentation
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>Summary by CodeRabbit
Improvements
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.