Repository navigation
Vue: Fix Unicode Default Values In Docs - #34909
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a normalization/validation layer for loose vue-component-meta-shaped test fixtures, introduces Unicode Vue3 story files and components plus an e2e test that verifies unicode defaults in docs, and bumps ChangesFixture normalization and test fixtures
Dependency Version Update
🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
|
Could a maintainer please add the |
It's done! Don't hesitate to reach out to us on https://discord.gg/invite/storybook if you need labels applied on a PR. You can also ping @valentinpalkovic or @Sidnioulz but we get a lot of email and sometimes miss notifications 🥴 I'll review after CI is done running. 🤞 |
9a30a72 to
05945d8
Compare
|
Actionable comments posted: 0 |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 21.88 MB | 21.68 MB | 🎉 -201 KB 🎉 |
| Dependency size | 36.44 MB | 36.44 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 109 | 102 | 🎉 -7 🎉 |
| Self size | 36 KB | 36 KB | 🎉 -24 B 🎉 |
| Dependency size | 44.15 MB | 42.99 MB | 🎉 -1.16 MB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 204 | 204 | 0 |
| Self size | 821 KB | 821 KB | 0 B |
| Dependency size | 91.26 MB | 91.06 MB | 🎉 -201 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 197 | 197 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 89.74 MB | 89.54 MB | 🎉 -201 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.09 MB | 1.09 MB | 0 B |
| Dependency size | 58.32 MB | 58.12 MB | 🎉 -201 KB 🎉 |
| Bundle Size Analyzer | node | node |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/renderers/vue3/src/docs/tests-meta-components/meta-components.ts (1)
8-16: 🏗️ Heavy liftPrefer typed fixture shapes over
unknown+ record casts in the normalization pipeline.Most assumptions here are enforced only at runtime. Defining narrow fixture interfaces (for prop/event/slot/expose entries and schema variants) and using typed predicates would move failures to compile time and reduce unsafe casts.
As per coding guidelines, "Encode assumptions with static checks first using TypeScript types and existing lint rules".
Also applies to: 45-46, 72-75, 82-90, 98-104, 111-113, 115-126, 128-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/renderers/vue3/src/docs/tests-meta-components/meta-components.ts` around lines 8 - 16, The LooseTestComponent type and its __docgenInfo fields currently use broad unknown[] types; create narrow interfaces for PropEntry, EventEntry, SlotEntry, ExposedEntry and union/schema types for the different docgen variants, replace the unknown[] arrays in LooseTestComponent.__docgenInfo with these specific typed arrays, and update the normalization pipeline functions (where LooseTestComponent is consumed) to use small type predicates/type guards that narrow runtime shapes before casting so failures surface at compile time rather than via unsafe casts; ensure to update usages around the existing symbols LooseTestComponent and __docgenInfo and any normalization helpers to accept and validate the new typed fixtures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@code/renderers/vue3/src/docs/tests-meta-components/meta-components.ts`:
- Around line 8-16: The LooseTestComponent type and its __docgenInfo fields
currently use broad unknown[] types; create narrow interfaces for PropEntry,
EventEntry, SlotEntry, ExposedEntry and union/schema types for the different
docgen variants, replace the unknown[] arrays in LooseTestComponent.__docgenInfo
with these specific typed arrays, and update the normalization pipeline
functions (where LooseTestComponent is consumed) to use small type
predicates/type guards that narrow runtime shapes before casting so failures
surface at compile time rather than via unsafe casts; ensure to update usages
around the existing symbols LooseTestComponent and __docgenInfo and any
normalization helpers to accept and validate the new typed fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c39edb9-01f6-4ff1-b851-b8499dbbc678
📒 Files selected for processing (1)
code/renderers/vue3/src/docs/tests-meta-components/meta-components.ts
Sidnioulz
left a comment
There was a problem hiding this comment.
Code LGTM! Thanks!
But we need a test added to the project to prevent regressions and prove the error is fixed within Storybook.
You could achieve this by adding autodocs stories to the Vue3 framework (folder code/renderers/vue3/template/stories_vue3-vite-default-ts/) with Unicode characters in withDefaults and defineProps. Then, an end-to-end test in code/e2e-tests/framework-vue3.spec.ts can go to the docs page for those stories and read the ArgsTable content to prove it is well parsed.
Thanks
|
@Sidnioulz Added the requested regression coverage in
Local checks run:
|
|
Thanks @Arunsiva003! One of the E2E test cases doesn't work because the docgen logic itself fails to compute default values. Don't change anything yet though. I'm gonna find some time to change the docgen algorithm in some of our sandboxes, and I'll keep your test and run it on the sandboxes that use the docgen algorithm which supports destructured props. I'll finish this PR afterwards. |
|
Waiting for #35071 to be merged and then we should be able to refactor this PR and get your tests passing. |
76d1b92 to
28f1481
Compare
|
I'll merge this without waiting for the other PR as we are going to delay it a little bit. Thanks for your patience! |
Closes #34893
What I did
Updated
@storybook/vue3-viteto usevue-component-meta^3.2.7so Vue prop default metadata preserves non-ASCII string defaults in generated docs. Refreshed the lockfile; the resolved version is3.3.2.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Ran a direct metadata smoke check against
vue-component-metawith Vue SFC defaults"こんにちは"and"大きい"; the metadata returned the original strings instead of Unicode escape sequences.Commands run:
XDG_CACHE_HOME=/tmp node .yarn/releases/yarn-4.10.3.cjs nx compile vue3-viteXDG_CACHE_HOME=/tmp NX_NO_CLOUD=true node .yarn/releases/yarn-4.10.3.cjs nx check vue3-vite --excludeTaskDependenciesXDG_CACHE_HOME=/tmp NX_NO_CLOUD=true node .yarn/releases/yarn-4.10.3.cjs exec vitest run --config code/renderers/vue3/vitest.config.ts code/renderers/vue3/src/extractArgTypes.test.tsDocumentation
MIGRATION.MD
No documentation update is needed because this is a dependency bug fix.
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
New Features
Tests
Chores