Skip to content
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: Improve argTypes inference with svelte2tsx - support runes #29423

Merged
merged 26 commits into from
Oct 24, 2024

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Oct 22, 2024

This is a copy of #28492 by @ciscorn, only done to be able to apply changes and release with Storybook 8.4.0. See #28492 (review)

All props goes to @ciscorn, they've done all the work here. 🙏🙏🙏

What I did

To support Svelte 5 'runes mode', this PR modifies @storybook/svelte-vite to utilize Svelte’s official svelte2tsx + typescript instead of sveltedoc-parser for generating props documentation data.

I found that others have mentioned the same idea as well:

Rationale:

  • sveltedoc-parser cannot parse Svelte 5 ‘runes mode’ files at all, and it has not been maintained for 3 years.
    • In contrast, svelte2tsx is an official component used by Svelte in its language server.
  • By using typescript, it provides perfect type resolution and can also handle type hints written in JavaScript + JSDoc as well as TypeScript.
  • It can handle both Svelte 5's 'runes mode' and the Svelte 4's export let style.
  • It works fast enough. (Around 30ms per stories on my laptop)

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. Open Svelte 4 (svelte-kit/skeleton-ts) and Svelte 5 (svelte-kit/prerelease-ts) sandboxes (feel free to use Chromatic)
  2. Navigate to the Docs of the following stories, and cross reference the Controls table with what is defined in the components, ensuring that the arg types are inferred correctly:

example screenshot:

Screenshot 2024-07-19 at 22 56 15

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/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

Greptile Summary

This pull request improves argTypes inference for Svelte components in Storybook, focusing on supporting Svelte 5's 'runes' mode and enhancing type resolution.

  • Replaced sveltedoc-parser with svelte2tsx for better compatibility with Svelte 5 and improved type inference
  • Added new Svelte components and stories to demonstrate various prop types and documentation styles
  • Updated existing tests and added new ones to cover the changes in argTypes inference
  • Improved handling of complex types, including unions, intersections, and function types in the documentation generation process
  • Retained backward compatibility for Svelte 4 components while adding support for Svelte 5 features

@JReinhold JReinhold self-assigned this Oct 22, 2024
@JReinhold JReinhold added the ci:daily Run the CI jobs that normally run in the daily job. label Oct 22, 2024
@JReinhold JReinhold marked this pull request as ready for review October 22, 2024 10:51
Copy link

nx-cloud bot commented Oct 22, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 08c2bb9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

29 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

return;
}

const isTsFile = /<script\s+[^>]*?lang=('|")(ts|typescript)('|")/.test(content);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Regex might not catch all TypeScript lang attributes. Consider using a parser

Suggested change
const isTsFile = /<script\s+[^>]*?lang=('|")(ts|typescript)('|")/.test(content);
import { parse } from 'some-parser-library';
const isTsFile = parse(content).hasTypeScriptLang();

export let number = 123;
/**
* True literal
* @type {true=}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: true= type annotation might be non-standard, consider using boolean=

Suggested change
* @type {true=}
* @type {boolean}

* Event callback function
* @type {(event: MouseEvent) => number}
*/
export let func = () => 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: func prop default value doesn't use event parameter

Suggested change
export let func = () => 10;
export let func = (event) => { return 10; };

@JReinhold
Copy link
Contributor Author

JReinhold commented Oct 22, 2024

I benchmarked the SvelteKit Prerelease sandbox:

diff:

Metric NEXT BRANCH Difference
devManagerIndexVisible 335ms 309ms -26ms
devStoryVisible 348ms 310ms -38ms
devStoryVisibleUncached 448ms 470ms +22ms
devAutodocsVisible 273ms 284ms +11ms
devMDXVisible 282ms 1.2s +918ms
buildManagerHeaderVisible 266ms 300ms +34ms
buildManagerIndexVisible 283ms 320ms +37ms
buildStoryVisible 291ms 320ms +29ms
buildAutodocsVisible 265ms 288ms +23ms
buildMDXVisible 280ms 280ms 0ms
buildTime 14.5s 16.4s +1.9s
buildSize 7.57 MB 7.61 MB +0.04 MB
buildSbAddonsSize 1.37 MB 1.37 MB 0 MB
buildSbCommonSize 209 kB 209 kB 0 kB
buildSbManagerSize 1.85 MB 1.85 MB 0 MB
buildSbPreviewSize 274 kB 274 kB 0 kB
buildStaticSize 0 B 0 B 0 B
buildPrebuildSize 3.7 MB 3.7 MB 0 MB
buildPreviewSize 3.87 MB 3.91 MB +0.04 MB
devPreviewResponsive 2.6s 2.8s +0.2s
devManagerResponsive 2.2s 2.4s +0.2s
createTime 3.6s 3.8s +0.2s
generateTime 10.6s 12.2s +1.6s
initTime 16.9s 15.8s -1.1s
createSize 0 B 0 B 0 B
generateSize 67.5 MB 67.5 MB 0 MB
initSize 319 MB 319 MB 0 MB
diffSize 252 MB 252 MB 0 MB

However the results are pretty flaky and I tried a few times. eg. devAUtodocsVisible could easily fluctuate between 300ms and 1.5s, same for devMDXVisible.

My conclusion is that we're fine.

@JReinhold JReinhold added the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Oct 24, 2024
@JReinhold JReinhold merged commit 4cf32b0 into next Oct 24, 2024
109 checks passed
@JReinhold JReinhold deleted the svelte-autodoc-ts branch October 24, 2024 13:27
@kasperpeulen kasperpeulen removed the needs qa Indicates that this needs manual QA during the upcoming minor/major release label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder-vite ci:daily Run the CI jobs that normally run in the daily job. feature request svelte sveltekit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants