Svelte: Fix docs generating when using lang="ts" or optional chaining

[j3rem1e]

Closes #24008 #24137

What I did

When sveltedoc-parser fails to parse a Svelte component, Storybook was not injecting the documentation metadata.
This metadata was used to determine the component name. If the information is missing, then we use component.name, which is not always good. For example, when using vite and HMR, svelte generate a proxy named Proxy<Component> and Storybook was displaying this name instead of the real component name.

This PR always injects documentation metadata, with the real name of the component (build at compile time).

Moreover, sveltedoc-parser is configured to parse ecmascript v9 and fails to parse elvis operator, or "??". This PR also change sveltedoc-parser to enable ecmascript v12. However, the configuration is not officially exposed so I have to patch the lib.. I have proposed a real PR to the author of the library.

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. Generate a svelte-vite sandbox
2. Start the storybook in dev mode
3. Modify "Button.svelte" and add "??" or "?." somewhere
4. The source generated for the stories should be valid. Without this PR, it shows <Proxy<Button> .../> instead.

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 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>

[j3rem1e]

@JReinhold The "sveltedoc-parser" plugin is duplicated between vite and webpack. I tried to mutualise the code, but I didn't know where I can put it. the "@storybook/svelte" is only build for a browser, and can't host code for the server.

[JReinhold]

Thanks for this! 💪

Could you add an e2e test for this?
Ideally it would be enough with a story to test this, but since this is docs related it's more involved than that, because Chromatic doesn't capture docs.

To do this:

1. Add a stories-file with autodocs to https://github.com/storybookjs/storybook/tree/next/code/renderers/svelte/template/stories that demonstrates this behavior.

2. You can test out the stories you're writing by generating a sandbox with yarn task --task sandbox --template svelte-vite/default-ts, it should symlink your stories into the sandbox.

3. Add an e2e test file at code/e2e-tests/framework-svelte-vite.spec.ts that visits the docs page and asserts that the source content is correct. You can see how this is done for NextJS here (the important part is to skip if the framework doesn't match). https://github.com/storybookjs/storybook/blob/next/code/e2e-tests/framework-nextjs.spec.ts

Let me know if this guidance isn't enough for you to move forward with it, or if you need me to write a test for this instead.

The "sveltedoc-parser" plugin is duplicated between vite and webpack

That's okay, that's just how it is structured, I don't think it's a big deal, but thanks for considering it. The best we can do is to link between the two files with a comment, so that if it's updated one place we remember to update it both places.

[j3rem1e]

I had to remove the e2e test of the documentation for typescript sandbox.

vite uses esbuild which removes jsdoc from the component - the doc is then not visible by the parser.
And it's not possible today to use vitePreprocess without esbuild. The documentation works when I use the "old" svelte-prepocess instead of vitePreprocess.

@benmccann maybe you know a workaround for this ?

[benmccann]

Hmm. Maybe we will need to tell people to use svelte-preprocess rather than vitePreprocess when using Storybook? Unless @dominikg or @bluwy have any other ideas

[dominikg]

Hmm. Maybe we will need to tell people to use svelte-preprocess rather than vitePreprocess when using Storybook? Unless @dominikg or @bluwy have any other ideas

that would not be good. they don't work the same so output can differ.

why exactly is the name parsed from a comment in the output at all? 99.9% it's sufficient to derive it from the filename (that's what svelte is doing too).

And for the other 0.1, you could just have an option?

[j3rem1e]

The issue is not about the name.

We use jsdoc to extract documentation about the properties, slots, events and the component description. And metadata as @slot and @wrapper.

And esbuild removes all comments.

[j3rem1e]

We should maybe only preprocess for typescript with svelte-preprocess in the plugin extracting the documentation.

[bluwy]

I'm not familiar with the flow, but why are we extracting the docs after processing? Shouldn't it extract the docs before that as the user authored? For example, if the Svelte component property is typed export let foo: string, preprocessing strips the string type away. If the string type were to be "saved" somewhere, maybe the component name could be placed there too?

[dominikg]

I don't know if it's easy to autodetect/autoconfigure the preprocessors.. However I have added in the PR a docPreprocess options in svelte.config.js to configure it.

is this a typo or are you expanding svelte.config.js with your own keys?

[j3rem1e]

I don't know if it's easy to autodetect/autoconfigure the preprocessors.. However I have added in the PR a docPreprocess options in svelte.config.js to configure it.

is this a typo or are you expanding svelte.config.js with your own keys?

I suppose you ask because I shouldn`t do that ? What the best way to configure this preprocessor ?
Before, the configuration was in storybook/main.js but it has be moved into svelte.config.js

[benmccann]

Why does the preprocessor even need to be configured? I'd remove the options altogether. vitePreprocess really only has options for on or off: https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/preprocess.md#vitepreprocess

[j3rem1e]

Why does the preprocessor even need to be configured? I'd remove the options altogether. vitePreprocess really only has options for on or off: https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/preprocess.md#vitepreprocess

I have added a possibility to configure it to handle edge-cases discuted here, like "what if the component is written with mdsvex", but I can remove it.

[socket-security]

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

[yannbf]

Hey @JReinhold would you mind to check this out whenever you have the time? Thanks!

[JReinhold]

This is great! Thanks for the hard work and the patience! ❤️

(just waiting for the CI to finish)

[JReinhold]

@j3rem1e CI shows that this breaks for non-TS projects, because the preprocessor will try to import typescript and fail. I did a lot of investigation and solved it in this commit: 270c94d
could you cherry pick it onto this branch, and see if you're okay with it? git cherry-pick 270c94da50270ca120fedee3ca207ba685a09c23

To summarize my findings:

1. Vite failed when trying to run the docgen plugin, with typescript missing module.
2. The creation of the plugin didn't fail in our sandboxes, because the dependencies are linked and because svelte-vite has a devDependency on typescript, it would actually be present, but it wouldn't be present to Vite during transformation. Therefore I moved the logic down into the plugin, instead of before it (ensuring that it still only runs once)
3. We can use require.resolve('typescript'), to check if typescript is present - @benmccann will this be an issue in Vite 5? should it be import.meta.resolve instead?
4. The JS sandboxes couldn't render the TypeScript button at all, which makes sense. So instead I reverted the button to be JavaScript, and then added a separate TypeScript button that is only added to the TS sandboxes (svelte-vite/default-ts and svelte-kit/skeleton-ts) - and updated the E2E tests accordingly.

[j3rem1e]

It's look ok to me.

Sorry for that, I though that typescript was a required dependency with storybook, I didn't check more.

[benmccann]

We can use require.resolve('typescript'), to check if typescript is present - @benmccann will this be an issue in Vite 5? should it be import.meta.resolve instead?

probably

[JReinhold]

I took a stab at import.meta.resolve, but import.meta is undefined. Maybe this will be fixed in #24395, either way I think require.resolve is fine for now.