-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Svelte: Fix union types generating invalid labels in argTypes #31980
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
Svelte: Fix union types generating invalid labels in argTypes #31980
Conversation
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 reviewed, no comments
Edit PR Review Bot Settings | Greptile
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.
2 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
| interface Props { | ||
| /** Is this the principal call to action on the page? */ | ||
| primary?: boolean; | ||
| variant?: 'primary' | 'secondary' | 'flat'; |
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.
style: Consider adding @default 'primary' in the JSDoc since there's a default value in the code
| const variantToClass = { | ||
| primary: 'storybook-button--primary', | ||
| secondary: 'storybook-button--secondary', | ||
| flat: '', |
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.
style: Empty string for 'flat' variant might be unclear - consider adding a comment explaining why flat variant has no class
|
|
||
| <!-- More on writing stories with args: https://storybook.js.org/docs/writing-stories/args --> | ||
| <Story name="Primary" args={{ primary: true, label: 'Button' }} /> | ||
| <Story name="Primary" args={{ variant: 'primary', label: 'Button' }} /> |
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.
logic: Verify that Button.svelte component accepts 'variant' prop, as this was changed from 'primary' without corresponding component changes visible
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.
There were component changes. Try not to hallucinate please.
| <Story name="Primary" args={{ primary: true, label: 'Button' }} /> | ||
| <Story name="Primary" args={{ variant: 'primary', label: 'Button' }} /> | ||
|
|
||
| <Story name="Secondary" args={{ label: 'Button' }} /> |
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.
logic: Secondary story should explicitly set variant='secondary' for consistency with Primary story's pattern
…storybook into fix-svelte-extract-arg-types
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 51 | 51 | 0 |
| Self size | 31.76 MB | 31.75 MB | 🎉 -16 KB 🎉 |
| Dependency size | 17.43 MB | 17.43 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
sb
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 52 | 52 | 0 |
| Self size | 1 KB | 1 KB | 0 B |
| Dependency size | 49.20 MB | 49.18 MB | 🎉 -16 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 217 | 217 | 0 |
| Self size | 582 KB | 582 KB | 0 B |
| Dependency size | 93.68 MB | 93.66 MB | 🎉 -16 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 186 | 186 | 0 |
| Self size | 31 KB | 31 KB | 0 B |
| Dependency size | 77.77 MB | 77.75 MB | 🎉 -16 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
|
I'll look into these CI/CD fails later today. |
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.
Thanks for the PR!
Indeed you are right, the added labels is wrong, and I have confirmed that removing them fixes the original issue with a "reverse" literal.
However we don't want to change any stories here. These stories are the default stories that all users get when creating a Storybook. For clarity and consistency, we want them to be the same across all our frameworks, so changing the Svelte stories to be a button with a variant prop would misalign it with all the other frameworks.
And we actually don't need to! We have heavy E2E testing for this, as we have stories that include every type of prop imaginable, where we can see the actual arg types being generated and the controls it generates:
- story on
next: https://630873996e4e3557791c069c-njbbynlcki.chromatic.com/?path=/story/stories-frameworks-svelte-vite-svelte-vite-default-ts-docgen-ts--default - story on your branch: https://630873996e4e3557791c069c-cxyxwmrmbz.chromatic.com/?path=/story/stories-frameworks-svelte-vite-svelte-vite-default-ts-docgen-ts--default
In the future, to properly test extractArgTypes, Storybook may want to consider using the internal doc generator instead of sveltedoc-parser.
I agree, the unit tests here could use an update, as it relies on the old way of extracting arg types.
So if you update the PR to only include the change in extarctArgTypes.ts, then we can merge it :)
|
Also, this doesn't close #30142 as that is a general problem for all frameworks, while this PR only changes the Svelte. So I've updated the PR description to not close that issue on merge. |
|
View your CI Pipeline Execution ↗ for commit 477ca9e
☁️ Nx Cloud last updated this comment at |
|
@JReinhold Thank you for the informative and constructive review. I've made the suggested changes. |
JReinhold
left a comment
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 ✨
Svelte: Fix union types generating invalid labels in argTypes (cherry picked from commit 628f30c)
Relates to #30142
What I did
When extracting the type args, props with union types were creating a labels array. Labels should be key-value maps that map an option to a new label. I compared to the other renderers and they don't generate labels at all from union types. I'm assuming it was a mistake. If that isn't the case I would love some direction.
In the tests for
extractArgTypesStorybook uses "sveltedoc-parser" which is insufficient. It doesn't support typescript or new svelte features. When running storybook, it uses an internal docgen tool here to generate the type structure. In the future, to properly testextractArgTypes, Storybook may want to consider using the internal doc generator instead of sveltedoc-parser.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task --task sandbox --start-from auto --template svelte-vite/default-tsfunction flat() { [native code] }Documentation
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 canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
Fixes Svelte union type extraction for improved Controls panel display in Storybook, replacing label arrays with proper key-value maps.
primaryprop to tri-statevariantprop ('primary', 'secondary', 'flat') for better control optionsfunction flat() { [native code] }variantToClassmapping for cleaner CSS class assignment based on variant selection