-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
Svelte: Ignore inherited HTMLAttributes
docgen when using utility types
#32173
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: Ignore inherited HTMLAttributes
docgen when using utility types
#32173
Conversation
Thanks for your PR! I'm tagging @JReinhold to take a look once he is available. |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
This is interesting work and an interesting problem. It looks pretty harmless to me, I'm tempted to merge as is, as it doesn't seem like it would break anything. I'm curious if @jasonlyu123 have any thoughts on this? |
WalkthroughThe docgen plugin’s prop-ignore logic now falls back to a syntheticOrigin valueDeclaration when valueDeclaration is missing, ensuring props originating from node_modules/svelte/elements.d.ts are still detected and skipped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as Docgen Plugin
participant T as TS Prop Node
participant D as Declaration Resolver
Note over P,T: Prop filtering during docgen
P->>T: Read prop.valueDeclaration
alt Declaration exists
T-->>P: valueDeclaration
else No direct declaration
P->>T: Check links.syntheticOrigin.valueDeclaration
T-->>P: syntheticOrigin.valueDeclaration (if present)
end
P->>D: Get source file for resolved declaration
D-->>P: fileName
alt fileName includes "node_modules/svelte/elements.d.ts"
Note over P: Ignore prop
P-->>P: Skip adding prop to docs
else
Note over P: Keep prop
P-->>P: Include prop in docs
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/frameworks/svelte-vite/src/plugins/generateDocgen.ts (1)
453-456
: Prefer public API before TS internalsBefore using the internal
links.syntheticOrigin
(unstable across TS versions), tryprop.declarations?.[0]
as a safer fallback.Consider:
- const __decl = - (prop.valueDeclaration ?? - (prop as any)?.links?.syntheticOrigin?.valueDeclaration) as ts.Declaration | undefined; + const __decl = + (prop.valueDeclaration ?? + prop.declarations?.[0] ?? + (prop as any)?.links?.syntheticOrigin?.valueDeclaration) as ts.Declaration | undefined;Please verify on TS versions you support (e.g., 5.4–5.6) to ensure behavior is consistent when props are created via utility types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/frameworks/svelte-vite/src/plugins/generateDocgen.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
View your CI Pipeline Execution ↗ for commit 21bc3d3
☁️ Nx Cloud last updated this comment at |
HTMLAttributes
when using utility types on svelte/elements
in docgen
HTMLAttributes
when using utility types on svelte/elements
in docgenHTMLAttributes
docgen when using utility types on svelte/elements
HTMLAttributes
docgen when using utility types on svelte/elements
HTMLAttributes
docgen when using utility types
…rs-doc Svelte: Ignore inherited `HTMLAttributes` when using utility types on `svelte/elements` in docgen (cherry picked from commit 756b758)
Demo of a workaround for: #32171
What I did
As described in the issue, when a component's props are defined using a TypeScript utility type like
Omit<HTMLAnchorAttributes, 'children'>
, autodoc fails to identify these props as originating fromsvelte/elements
. This causes the automatic documentation to be flooded with all inherited HTML attributes.During some debugging, I found that for props derived from a utility type, the
prop.valueDeclaration
property isundefined
and the existing check fails. However, it seems like the TypeScript compiler retains a link to the original type inprop.links.syntheticOrigin.valueDeclaration
.This PR shows how falling back to this deeper path skips doc generation for inherited props if
prop.valueDeclaration
is not present (at least for basic cases).This is intended merely as a starting point for the discussion, as accessing internal-looking properties like
links.syntheticOrigin
with the mighty help ofas any
probably isn't the best idea. Just thought this may be helpful.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
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>
Summary by CodeRabbit