Cleanup Svelte generator

[benmccann]

What I did

• Preprocessor should not be manually set in main.cjs. It's already set in svelte.config.js. Setting it manually results in duplication and is often wrong causing breakages when it does not match the user's own config (e.g. TypeScript and Preprocessors support addon-svelte-csf#4)
• svelte-loader should not be added indiscriminately. It is only needed for webpack projects, but Svelte projects use Vite by default, so it's mostly wrong to add it. Users will have set this up already if they're using Svelte and webpack

How to test

• Is this testable with Jest or Chromatic screenshots?
• Does this need a new example in the kitchen sink apps?
• Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

[nx-cloud]

☁️ Nx Cloud Report

We didn't find any information for the current pull request with the commit 50991a6.
You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.

---

Sent with 💌 from NxCloud.

[shilman]

@benmccann what's the best way to test these changes?

We should probably sync up on how this fits in with all the changes we're making in 7.0 in future/base. As it stands, I'm not super comfortable patching this back to 6.5--tho I could def be convinced!

For example, there's an architectural change since 6.4 that will allow a bunch of perf improvements and simplifications, but that is not currently compatible with addon-svelte-csf. I've created an issue here storybookjs/addon-svelte-csf#65. Until that's fixed and we have a good system for documenting the usage, I wouldn't be comfortable installing Svelte CSF by default.

[benmccann]

Ok, I've removed addon-svelte-csf for now. The other changes seem like they make sense to me regardless of whether you're running with 6.x or 7.x

You can create a new Svelte project with:

npm create vite@latest myapp -- --template svelte

You can create a new SvelteKit project with:

npm create svelte@latest myapp

There will need to be additional changes for full SvelteKit support.

[shilman]

Awesome @benmccann -- I'll try to kick the tires today! I'm so grateful to see all your improvements coming in. And with the 7.0 changes I think we'll be able to achieve a step change in DX for Vite-based projects .... 🚀

BTW, are you on our Discord? https://discord.gg/storybook Would love to chat when you have a min

[benmccann]

I pinged you on Discord. I have the same username there

[j3rem1e]

Just to understand, where are the duplication you are talking about ?

Preprocessors are used here to generate documentation : https://github.com/storybookjs/storybook/blob/next/app/svelte/src/server/svelte-docgen-loader.ts#L55

If the preprocessors are not configured correctly, it will not works for typescript projects ? Or the svelte compiler is autodetecting itself the preprocessor to use ?

[benmccann]

The user has already specified the preprocessors to use in svelte.config.js. Any tooling in the Svelte ecosystem is expected to read that file in order to load the preprocessors. If the preprocessors are not set the same in .storybook/main.cjs and in svelte.js then things will start to break, so I'm not sure I'd even give the user the ability to specify it differently. We should probably update the line you linked to so that it reads from svelte.config.js the same as builder-vite does as it's much easier for the user to use vs making them read the config file themselves

[j3rem1e]

We should probably update the line you linked to so that it reads from svelte.config.js the same as builder-vite does

builder-vite use the same preprocessor options:

https://github.com/storybookjs/builder-vite/blob/main/packages/builder-vite/svelte/csf-plugin.ts#L19
https://github.com/storybookjs/builder-vite/blob/main/packages/builder-vite/plugins/svelte-docgen.ts#L58

[benmccann]

Yes, but those options are read directly from the config file so that the user doesn't have to specify them a second time:
https://github.com/storybookjs/builder-vite/blob/a4f17ea35d16161a8dc77336213fee058346ad4b/packages/builder-vite/vite-config.ts#L147

[j3rem1e]

Maybe this logic should be implemented in the core storybook/app/svelte instead of the vite-builder ? This doesn't seem to be really related to vite.

[benmccann]

If each plugin has to load the Svelte config you're going to end up loading it more times. Here we load it once and then pass it to two plugins. So the way it's done now makes sense to me. But if the user were expected to create these plugins manually then I think it'd be better to encapsulate it inside the plugins so that the user is not responsible for loading the config file.

[j3rem1e]

What I was trying to say it's maybe the svelte.config.js should be loaded (once) by the storybook core svelte plugin and be available through svelteOptions like today. you don't have to change every plugins using it, and the logic will be shared by vite/webpack/whatever.

I understand that letting the user configure preprocessor can be a source of errors. however, removing this option and hardcoding the location of "svelte.config.js" is a breaking change - probably an acceptable breaking change? - but I have projects I'm sure it will not work without change/fake svelte.config.js)

[benmccann]

What I was trying to say it's maybe the svelte.config.js should be loaded (once) by the storybook core svelte plugin and be available through svelteOptions like today. you don't have to change every plugins using it, and the logic will be shared by vite/webpack/whatever.

I'm not sure I understand how that would work, but it's probably my lack of knowledge about the Storybook codebase. Does the Storybook Svelte plugin get invoked before the Vite/Webpack builder? How would it pass the options?

I understand that letting the user configure preprocessor can be a source of errors. however, removing this option and hardcoding the location of "svelte.config.js" is a breaking change - probably an acceptable breaking change? - but I have projects I'm sure it will not work without change/fake svelte.config.js)

Yeah, I did not remove the option to avoid the breaking change, but instead made it optional with the defaults loaded from the config file.

[IanVS]

Yeah, I did not remove the option to avoid the breaking change, but instead made it optional with the defaults loaded from the config file.

FWIW, I treated the change to start loading from the config file in the vite-builder as a breaking change, even though the config can be overridden from svelteOptions, since there may be configuration in svelte.config.js that the user does not want to be used, but with the change it starts to be. But maybe I'm being overly cautious.

I'm not sure I understand how that would work, but it's probably my lack of knowledge about the Storybook codebase. Does the Storybook Svelte plugin get invoked before the Vite/Webpack builder? How would it pass the options?

Storybook has a way for presets to add/modify config settings and then access them elsewhere using await presets.apply('configKey', overrides, options). I'm not 100% certain how it works, but I think the svelte plugin would export svelteOptions from its preset.ts file, similar to what it does right now for addons (

storybook/app/svelte/src/server/preset.ts

Line 3 in c9225ab

export const addons: StorybookConfig['addons'] = [

). Then, the vite builder can use await presets.apply('svelteOptions') to access the config loaded from svelte.config.js. Is that what you had in mind, @j3rem1e?

[benmccann]

Closing this in favor of #18799