Svelte: Improve argTypes inference with `svelte2tsx` - support runes #28492

ciscorn · 2024-07-09T09:39:00Z

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!

Open Svelte 4 (svelte-kit/skeleton-ts) and Svelte 5 (svelte-kit/prerelease-ts) sandboxes (feel free to use Chromatic)
- Svelte 4: https://www.chromatic.com/builds?appId=634ff6954afef8aae1c1b29b&branch=next
- Svelte 5: https://www.chromatic.com/builds?appId=65688a1304a0ce1c79fcf479&branch=next
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:
- /?path=/docs/stories-renderers-svelte-docs-ts--docs ↔️ https://github.com/MIERUNE/storybook/blob/svelte-autodoc-ts/code/frameworks/sveltekit/template/stories_svelte-kit-skeleton-ts/DocsTS.svelte
- /?path=/docs/stories-renderers-svelte-docs-jsdoc--docs ↔️ https://github.com/MIERUNE/storybook/blob/svelte-autodoc-ts/code/renderers/svelte/template/stories/DocsJSDoc.svelte
- SvelteKit Prerelease only: /?path=/docs/stories-renderers-svelte-docs-ts-inline-prop-types--docs ↔️ https://github.com/MIERUNE/storybook/blob/svelte-autodoc-ts/code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/DocsTSInlinePropTypes.svelte
- SvelteKit Prerelease only: /?path=/docs/stories-renderers-svelte-docs-ts-referenced-prop-types--docs ↔️ https://github.com/MIERUNE/storybook/blob/svelte-autodoc-ts/code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/DocsTSReferencedPropTypes.svelte
- SvelteKit Prerelease only: /?path=/docs/stories-renderers-svelte-docs-jsdoc-runes--docs ↔️ https://github.com/MIERUNE/storybook/blob/svelte-autodoc-ts/code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/DocsJSDocRunes.svelte

example screenshot:

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

This pull request has been released as version 0.0.0-pr-28492-sha-08d3f4f1. Try it out in a new sandbox by running npx [email protected] sandbox or in an existing project with npx [email protected] upgrade.

More information


Published version	`0.0.0-pr-28492-sha-08d3f4f1`
Triggered by	@JReinhold
Repository	MIERUNE/storybook
Branch	`svelte-autodoc-ts`
Commit	`08d3f4f1`
Datetime	Thu Oct 17 19:11:04 UTC 2024 (`1729192264`)
Workflow run	11391471758

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=28492

greptile-apps

PR Summary

Updated code/frameworks/svelte-vite/package.json to add svelte2tsx and update typescript dependency.
Modified code/frameworks/svelte-vite/src/plugins/svelte-docgen.ts to switch from sveltedoc-parser to svelte2tsx and TypeScript.
Added code/frameworks/svelte-vite/src/plugins/ts2doc.ts to convert Svelte files to TypeScript documentation.
Introduced code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/ButtonTypeScriptRunes.svelte for Svelte 5 'runes mode' with TypeScript support.
Added code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/ts-runes-docs.stories.js for Storybook story of ButtonTypeScriptRunes component.

_{5 file(s) reviewed, 3 comment(s)}
_{Edit PR Review Bot Settings}

code/frameworks/svelte-vite/src/plugins/ts2doc.ts

.../frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/ButtonTypeScriptRunes.svelte

JReinhold

This is very interesting work! 🎉

As you can see from my comments, I'm eager to know if we can just switch completely to using svelte2tsx for all cases and not just in runes mode? Even for Svelte 4 and 5?

I haven't been able to dive into the details of the node visiting code yet.

I also would like to see the test stories test out more cases:

a prop being typed by an imported type
another component that doesn't have lang="ts" but instead only uses JSDoc comments only.

code/frameworks/svelte-vite/package.json

code/frameworks/svelte-vite/src/plugins/svelte-docgen.ts

code/frameworks/svelte-vite/src/plugins/ts2doc.ts

nx-cloud · 2024-07-10T08:38:15Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 08d3f4f. 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

nx run-many -t build --parallel=3

Sent with 💌 from NxCloud.

jasonlyu123 · 2024-07-12T01:57:44Z

Hi. svelte2tsx maintainer here. svelte2tsx uses TypeScript API to parse script tag content so I don't think it'll work if you don't have TypeScript. Also, we don't recommend using AST static analysis for props type. exportedNames is there because we previously did that but later switched to the TypeScript type-checker. If you don't want TypeScript to be a dependency, maybe you'll need to make the original version a fallback.

Another problem with the approach is that the $$ComponentProps type won't be declared if the entire props, not a single one, is annotated with the named type. So you can't rely on that to determine if this is rune mode. This is also another reason why static analysis is not reliable, The named type might be imported from another file.

JReinhold · 2024-07-12T08:56:32Z

I don't think it'll work if you don't have TypeScript.

Thanks for your valuable input! ❤️ Then I think we should just keep typescript in, with the widest possible version range.

This PR fixes a top 3 most requested feature for Svelte support.
80% of users are using TypeScript anyway, so they'll have it installed
It won't ever be part of the final bundle, so it's "only" a problem when installing dependencies.

@shilman @kasperpeulen feel free to argue against me here, given we are very actively working on reducing Storybook's dependency graph where possible.

@jasonluy given you're a svelte2tsx maintainer I'd love for any other feedback from you on this approach, or a code review if possible. You might see stuff that we're missing here.

ciscorn · 2024-07-12T09:32:13Z

@jasonlyu123, thank you so much!

After rewriting to use @babel/parser, I realized that svelte2tsx depends on typescript (even though the package.json of svelte2tsx says it is a devDependency). I'll need to roll back to using TypeScript.

$$ComponentProps type won’t be declared if the entire props, not a single one, is annotated with the named type

I just realized this myself a moment ago :)

The named type might be imported from another file.

AFAIK, react-docgen also handles only simple cases. I think we have to make some compromises as well.

shilman · 2024-07-12T09:40:35Z

AFAIK, react-docgen also handles only simple cases. I think we have to make some compromises as well.

Compared to tsc, react-docgen only handles simple cases. However, it has the concept of a "resolve" so types imported from other paths should work, even if those paths rely on TS aliases. Thus it's much more usable than it has been in the past.

benmccann · 2024-07-15T16:28:47Z

I realized that svelte2tsx depends on typescript (even though the package.json of svelte2tsx says it is a devDependency).

I checked to make sure we're doing the right thing in svelte2tsx and it looks like Typescript is listed in peerDependencies

ciscorn · 2024-07-15T20:52:25Z

I checked to make sure we're doing the right thing in svelte2tsx and it looks like Typescript is listed in peerDependencies

Sorry. I was confused by the Yarn used by Storybook. Yarn does not automatically install peerDependencies.

However, if svelte2tsx directly depends on TypeScript, wouldn't it be better to specify it in dependencies? (I'm not very familiar with Node's package resolution mechanism, though.)

jasonlyu123

I am not quite familiar with storybook so I didn't dive deep into the integration. But I can give some general suggestions with TypeScript and svelte2tsx API you can explore.

jasonlyu123 · 2024-07-27T01:04:36Z

code/frameworks/svelte-vite/src/plugins/generateDocgen.ts

+  }
+  let propsRuneUsed = false;
+
+  const options = loadCompilerOptions(targetFileName);


Parse tsconfig also scans the file system for the files included in the tsconfig include config. It's returned in the ParsedCommandLine.fileNames. It might be quite heavy. It might be worth watching the file and cache the result.

The createProgram is also heavy so might be better in a larger codebase to reuse the problem. You can probably try creating the program with ParsedCommandLine.fileNames but maybe only the svelte files since the ts/js file might never be used. When the targetFileName is not in the list or file content changes then recreate the program. It's also worth exploring passing the old program to createProgram and TypeScript will try to reuse the last program.

Thank you very much for your suggestions! I added some caching mechanisms:

If the previously loaded ParsedCommandLine.fileNames includes the target .svelte file, it doesn't reload the tsconfig.

Passing the previous program as oldProgram to createProgram. Subsequent calls to createProgram take less than 10ms for the Storybook sveltekit/prerelease-ts example on my machine.

I believe these measures ensure adequate performance. Watching for file changes seems difficult for me and might be too heavy to implement in this PR. What do you think? @jasonlyu123 @JReinhold

You can reference the file watching here.
https://github.com/vitejs/vite/blob/8d0a9c1ab8ddd26973509ca230b29604e872e2cd/packages/vite/src/node/plugins/esbuild.ts#L110

One thing I am also curious about is that sometimes the user starts the dev server when svelte-kit hasn't generated the ./svelte-kit/tsconfig.json file. Does that affect the amount of svelte files being loaded? or maybe the plugin will be loaded after the svelte-kit plugin so it's fine? If it does, maybe the file watch is necessary.

code/frameworks/svelte-vite/src/plugins/generateDocgen.ts

JReinhold

Great work! Added a few minor comments, and there's still some left from previous reviews.

stories look great!

code/frameworks/svelte-vite/src/plugins/generateDocgen.ts

code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/DocsTSRunes2.svelte

code/frameworks/sveltekit/template/stories_svelte-kit-prerelease-ts/DocsJSDocRunes.svelte

code/renderers/svelte/src/docs/extractArgTypes.ts

ciscorn · 2024-09-07T02:12:46Z

@JReinhold Sorry for the delay. I have reflected your review!

benmccann · 2024-09-12T17:46:53Z

If sveltedoc-parser is able to be removed then it would remove about 100 dependencies from the dependency graph, which is another thing @JReinhold has been looking at. https://npmgraph.js.org/?q=sveltedoc-parser

JReinhold · 2024-09-12T21:12:52Z

If sveltedoc-parser is able to be removed then it would remove about 100 dependencies from the dependency graph, which is another thing @JReinhold has been looking at. https://npmgraph.js.org/?q=sveltedoc-parser

Unfortunately that would be a breaking change because sveltedoc-parser is slightly better when it comes to slots, see #28492 (comment)

I don't know how many actually use that feature (if any), but I have a hard time justifying dropping the support in a minor, even though I want to.

We should definitely remove it in 9.0 though.

benmccann · 2024-09-12T22:27:41Z

I wonder if we need the typescript and svelte2tsx dependencies. I'm not an expert in this area, but passing along a note from @dummdidumm:

Comments are returned as part of the AST. You would "just" need to analyze the AST afterwards to find the correct comments. With $props this should be a lot more straightforward. And since Svelte understands TS natively now you also don't need to switch to something else for that (which would've been a big selling point of svelte2tsx before)

I expect that this is perhaps only possible with Svelte 5. Either way, this is probably an improvement as it is, but if we could remove the need for those dependencies as well longer-term certainly that would be nice

JReinhold

🎉

JReinhold

There's actually a regression, see the Chromatic snapshot for argtypes inference:

https://www.chromatic.com/test?appId=6308736456ad2046275c0ae7&id=66dbf220a2751a07f5d4fdc2

It's not easy to spot, but for objects, it changes slightly from:

  "style": {
    "control": {
      "type": "object"
    },
    "name": "style",
    "description": "Styles to use for this component",
    "type": {
      "required": false,
      "name": "object" 👈 correct
    },
    "table": {
      "type": {
        "summary": "object"
      },
      "defaultValue": {},
      "category": "properties"
    }
  },

to:

  "style": {
    "control": {
      "type": "object"
    },
    "name": "style",
    "description": "Styles to use for this component",
    "type": {
      "required": false,
      "name": "{}" 👈 incorrect
    },
    "table": {
      "type": {
        "summary": "{}"
      },
      "defaultValue": {
        "summary": "{}"
      },
      "category": "properties"
    }
  },

The rest of the changes looks good, they just appear to be more correct than what we had before, but that name change is wrong because the Storybook API expects name: 'object':

https://storybook.js.org/docs/api/arg-types#type

I think we can just special case it here:

https://github.com/MIERUNE/storybook/blob/08d3f4f186d18dcca5ecf3c30e030f90a063a81d/code/renderers/svelte/src/docs/extractArgTypes.ts#L45

With something like

name: item.type?.text === '{}' ? 'object' : item.type?.text as SBScalarType['name'],

If you give me edit access @ciscorn I can help get this PR over the finish line, I'd love to merge it in the coming days. 🙏
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

JReinhold · 2024-10-18T08:56:09Z

I've fixed the issue reported above and fixed merge conflicts locally, now I just need permission to push to the branch. 🙏

The feature freeze for 8.4.0 goes into effect on Wednesday, Oct 23, so we need to get it in before then.

JReinhold · 2024-10-24T13:28:45Z

GitHub UI is a bit weird here, this PR didn't actually get "merged", but the other PR #29423 that I created included all the commits from here, and that got merged, which confuses GitHub slightly.

storybook-bot · 2024-11-22T21:49:39Z

Failed to publish canary version of this pull request, triggered by @JReinhold. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/11980658402

ciscorn force-pushed the svelte-autodoc-ts branch from 88733ed to 700862a Compare July 9, 2024 09:55

ciscorn changed the title ~~Svelte: autodocs with svelte2tsx and TypeScript~~ Svelte: Autodocs with svelte2tsx and TypeScript (Supports 'runes mode') Jul 9, 2024

ciscorn marked this pull request as ready for review July 9, 2024 10:00

greptile-apps bot reviewed Jul 9, 2024

View reviewed changes

ciscorn force-pushed the svelte-autodoc-ts branch from 700862a to 9d05996 Compare July 9, 2024 10:09

svelte: autodocs with svelte2tsx

c7a619d

ciscorn force-pushed the svelte-autodoc-ts branch from 9d05996 to c7a619d Compare July 9, 2024 10:09

ciscorn mentioned this pull request Jul 9, 2024

Storybook support sveltejs/kit#5397

Closed

JReinhold added svelte builder-vite ci:daily Run the CI jobs that normally run in the daily job. labels Jul 10, 2024

JReinhold reviewed Jul 10, 2024

View reviewed changes

rename: ts2doc -> generateDocgen

ad9429b

ciscorn changed the title ~~Svelte: Autodocs with svelte2tsx and TypeScript (Supports 'runes mode')~~ Svelte: Autodocs with svelte2tsx (Supports 'runes mode') Jul 11, 2024

ciscorn marked this pull request as draft July 11, 2024 00:51

ciscorn added 2 commits July 12, 2024 02:38

svelte2tsx + babel/parser (remove typescript dep)

e38d5c0

Merge remote-tracking branch 'upstream/next' into svelte-autodoc-ts

a4f4a6a

ciscorn force-pushed the svelte-autodoc-ts branch from f1a2912 to a4f4a6a Compare July 11, 2024 18:07

svelte docgen: enrich stories

43bc7d5

ciscorn force-pushed the svelte-autodoc-ts branch from 95d5665 to 43bc7d5 Compare July 12, 2024 07:59

Migrate to TypeScript for parsing and type inference

b574f80

ciscorn marked this pull request as ready for review July 15, 2024 20:53

ciscorn requested a review from JReinhold July 15, 2024 20:53

jasonlyu123 reviewed Jul 27, 2024

View reviewed changes

ciscorn added 2 commits July 27, 2024 20:19

svelte autodocs: symbol, enum

82379df

svelte autodoc: improve jsdoc support

38a16a5

ciscorn force-pushed the svelte-autodoc-ts branch from ca36a55 to 38a16a5 Compare July 28, 2024 02:04

ciscorn added 2 commits July 29, 2024 14:43

svelte docgen: improve caching

2b7694d

svelte docgen: fix for e2e test

7e2d0f8

ciscorn force-pushed the svelte-autodoc-ts branch from 6654d3c to 7e2d0f8 Compare July 29, 2024 12:35

ciscorn requested review from JReinhold and jasonlyu123 July 29, 2024 12:48

JReinhold requested changes Aug 21, 2024

View reviewed changes

ciscorn added 4 commits September 7, 2024 02:12

non-string union into radio

5781385

prefer tsconfig over jsconfig

ece406c

svelte-kit/prerelease-ts: rename doc examples

5c994a0

Merge remote-tracking branch 'upstream/next' into svelte-autodoc-ts

1fe5ca8

ciscorn requested a review from JReinhold September 7, 2024 05:33

ciscorn force-pushed the svelte-autodoc-ts branch from 973702d to 7d5d81e Compare September 7, 2024 06:04

fix lint, e2e

08d3f4f

ciscorn force-pushed the svelte-autodoc-ts branch from 7d5d81e to 08d3f4f Compare September 7, 2024 06:09

JReinhold added the feature request label Oct 17, 2024

JReinhold approved these changes Oct 17, 2024

View reviewed changes

JReinhold requested changes Oct 17, 2024

View reviewed changes

JReinhold mentioned this pull request Oct 22, 2024

Svelte: Improve argTypes inference with svelte2tsx - support runes #29423

Merged

8 tasks

JReinhold merged commit 08d3f4f into storybookjs:next Oct 24, 2024
101 of 107 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Svelte: Improve argTypes inference with `svelte2tsx` - support runes #28492

Svelte: Improve argTypes inference with `svelte2tsx` - support runes #28492

ciscorn commented Jul 9, 2024 •

edited by JReinhold

Loading

greptile-apps bot left a comment

JReinhold left a comment

nx-cloud bot commented Jul 10, 2024 •

edited

Loading

jasonlyu123 commented Jul 12, 2024

JReinhold commented Jul 12, 2024

ciscorn commented Jul 12, 2024

shilman commented Jul 12, 2024 •

edited

Loading

benmccann commented Jul 15, 2024

ciscorn commented Jul 15, 2024 •

edited

Loading

jasonlyu123 left a comment

jasonlyu123 Jul 27, 2024 •

edited

Loading

ciscorn Jul 29, 2024

jasonlyu123 Aug 19, 2024

JReinhold left a comment

ciscorn commented Sep 7, 2024

benmccann commented Sep 12, 2024

JReinhold commented Sep 12, 2024

benmccann commented Sep 12, 2024

JReinhold left a comment

JReinhold left a comment •

edited

Loading

JReinhold commented Oct 18, 2024 •

edited

Loading

JReinhold commented Oct 24, 2024

storybook-bot commented Nov 22, 2024

Svelte: Improve argTypes inference with svelte2tsx - support runes #28492

Svelte: Improve argTypes inference with svelte2tsx - support runes #28492

Conversation

ciscorn commented Jul 9, 2024 • edited by JReinhold Loading

What I did

Rationale:

Checklist for Contributors

Testing

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

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

JReinhold left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Jul 10, 2024 • edited Loading

☁️ Nx Cloud Report

jasonlyu123 commented Jul 12, 2024

JReinhold commented Jul 12, 2024

ciscorn commented Jul 12, 2024

shilman commented Jul 12, 2024 • edited Loading

benmccann commented Jul 15, 2024

ciscorn commented Jul 15, 2024 • edited Loading

jasonlyu123 left a comment

Choose a reason for hiding this comment

jasonlyu123 Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

ciscorn Jul 29, 2024

Choose a reason for hiding this comment

jasonlyu123 Aug 19, 2024

Choose a reason for hiding this comment

JReinhold left a comment

Choose a reason for hiding this comment

ciscorn commented Sep 7, 2024

benmccann commented Sep 12, 2024

JReinhold commented Sep 12, 2024

benmccann commented Sep 12, 2024

JReinhold left a comment

Choose a reason for hiding this comment

JReinhold left a comment • edited Loading

Choose a reason for hiding this comment

JReinhold commented Oct 18, 2024 • edited Loading

JReinhold commented Oct 24, 2024

storybook-bot commented Nov 22, 2024

Svelte: Improve argTypes inference with `svelte2tsx` - support runes #28492

Svelte: Improve argTypes inference with `svelte2tsx` - support runes #28492

ciscorn commented Jul 9, 2024 •

edited by JReinhold

Loading

nx-cloud bot commented Jul 10, 2024 •

edited

Loading

shilman commented Jul 12, 2024 •

edited

Loading

ciscorn commented Jul 15, 2024 •

edited

Loading

jasonlyu123 Jul 27, 2024 •

edited

Loading

JReinhold left a comment •

edited

Loading

JReinhold commented Oct 18, 2024 •

edited

Loading