Vite: Add partial SvelteKit support #19338

JReinhold · 2022-10-03T23:17:19Z

Issue: #19280

This PR is a first stab at setting up a SvelteKit repro and getting it to work out of the box.

TODO

Test that base vite-svelte framework still works
Test that TS SvelteKit framework works.
Test that it works when published to verdaccio

What I did

Add repros via create-svelte-with-args
Removed the sveltePreprocess option from the template as that was causing ESM vs CJS issues and we decided it was an anti-pattern anyway since they should automatically be picked up by the Vite config
Remove vite-plugin-svelte-kit whenever it is recognised in a Vite config because it conflicts with Storybook's VIte server handling. This has some drawbacks that certain SvelteKit features aren't available in Storybook but for now we consider that a general issue that needs to be solved in SvelteKit.

How to test

git checkout origin/jeppe/sb-544-sveltekit
Use Node 16 instead of Node 14 (SvelteKit can't be built using v14) - eg. nvm use 16
Generate repro with cd code && yarn generate-repros-next --template svelte-kit/skeleton-js
cd ../sandbox/svelte-kit-skeleton-js && yarn install
yarn storybook
See that all basic stories work.
Repeat for SvelteKit with TypeScript: svelte-kit/skeleton-ts

Out of scope

I want to include a few stories based on the Svelte CSF format in the sandbox so CI also ensures that they work. That's another PR.

JReinhold · 2022-10-03T23:19:41Z

code/frameworks/svelte-vite/src/preset.ts

@@ -24,8 +25,23 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets

  plugins.push(svelteDocgen(config));

+  removeSvelteKitPlugin(plugins);


Ideally this should have been a plugin, but it doesn't seem to have an affect. The plugin correctly removes vite-plugin-svelte-kit from the config but it doesn't change any behavior

Yes unfortunately there's no way to remove plugins from within a plugin, as far as I can tell.

code/lib/builder-vite/src/vite-config.ts

code/lib/builder-vite/src/vite-server.ts

JReinhold · 2022-10-03T23:24:06Z

code/lib/cli/src/generators/SVELTE/index.ts

-  let extraMain;
-  // svelte.config.js ?
-  if (fse.existsSync('./svelte.config.js')) {
-    logger.info("Configuring preprocessor from 'svelte.config.js'");
-
-    extraMain = {
-      svelteOptions: { preprocess: '%%require("../svelte.config.js").preprocess%%' },
-    };
-  } else if (fse.existsSync('./svelte.config.cjs')) {
-    logger.info("Configuring preprocessor from 'svelte.config.cjs'");
-
-    extraMain = {
-      svelteOptions: { preprocess: '%%require("../svelte.config.cjs").preprocess%%' },
-    };
-  } else {
-    // svelte-preprocess dependencies ?
-    const packageJson = packageManager.retrievePackageJson();
-    if (packageJson.devDependencies && packageJson.devDependencies['svelte-preprocess']) {
-      logger.info("Configuring preprocessor with 'svelte-preprocess'");
-
-      extraMain = {
-        svelteOptions: { preprocess: '%%require("svelte-preprocess")()%%' },
-      };
-    }
-  }
-


as per #19280 (comment)

Does this need any MIGRATION.md notes? @JReinhold

You're right, users needs to remove their svelteOptions property in main.cjs and rely on their Vite config to pick them up instead. Svelte+Webpack users probably don't want to remove anything.

I'll write it up.

What do you think about pulling this change out to its own PR? I'd love to get it into the next release, since svelte apps are currently broken after bootstrapping with Storybook.

…-544-sveltekit

IanVS

Thanks for this! It'll be great to have sveltekit repros, and getting it working out-of-the-box is an important step to better svelte support!

code/frameworks/svelte-vite/src/preset.ts

code/lib/builder-vite/src/vite-config.ts

IanVS · 2022-10-04T01:31:49Z

code/frameworks/svelte-vite/src/preset.ts

@@ -24,8 +25,23 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets

  plugins.push(svelteDocgen(config));

+  removeSvelteKitPlugin(plugins);


Yes unfortunately there's no way to remove plugins from within a plugin, as far as I can tell.

IanVS · 2022-10-04T02:53:52Z

code/frameworks/svelte-vite/src/preset.ts

  return {
    ...config,
    plugins,
  };
 };
+
+const removeSvelteKitPlugin = (plugins: PluginOption[]) => {


I'd still love to find a way to avoid having to do this, since we'll start getting complaints about sveltekit aliases not working (even though there seems to be some debate over whether they should be used at all inside components). But, this seems like a reasonable workaround for now, until we can figure it out.

I agree, let's continue investigating the core issue and do a follow up to this.

code/lib/builder-vite/src/vite-server.ts

IanVS · 2022-10-04T13:33:06Z

code/frameworks/svelte-vite/src/preset.ts

@@ -24,7 +24,7 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets
 const removeSvelteKitPlugin = (plugins: PluginOption[]) => {
  plugins.forEach((plugin, index) => {
    if (plugin && 'name' in plugin && plugin.name === 'vite-plugin-svelte-kit') {
-      // eslint-disable-next-line no-param-reassign -- we explicitly want to mutate the array as stated in Vite docs
+      // eslint-disable-next-line no-param-reassign -- we explicitly want to mutate the array as stated here: https://vitejs.dev/guide/api-plugin.html#config


Since this is not in a config hook, I think we don't necessarily need to follow that rule, but this seems like an okay approach anyway. I normally prefer map and filter, but since this is recursive, that might be a little trickier.

The Vite docs are a bit unclear on how it's handled when a partial config is returned - they say it is deeply merged into the existing config, which led me to believe it's not usable for removing any entries as we want to achieve here.

Yes, but those docs are for a plugin. This code is not inside a plugin, it's directly modifying the config that we'll end up passing to vite.createServer() inside vite-server.ts. So there's no merging here except for the kind that we do ourselves.

oh yeah, I hadn't even noticed that.
I have a version that is more declarative, but I'm unsure if it's actually more readable. But I don't have any strong opinions so if you think it's better I'll happily push it.

export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets }) => { const { plugins = [] } = config; return { ...config, plugins: [ // remove SvelteKit plugin ...plugins.map(withoutSvelteKitPlugin), // Add svelte plugin if not present !hasPlugin(plugins, 'vite-plugin-svelte') && (await import('@sveltejs/vite-plugin-svelte')).svelte(), // Add docgen plugin svelteDocgen(config), ].filter(Boolean), }; }; const withoutSvelteKitPlugin = (plugin: PluginOption): PluginOption => { if (Array.isArray(plugin)) { // recursive - Vite plugins can be nested return plugin.map(withoutSvelteKitPlugin).filter(Boolean); } if (plugin && 'name' in plugin && plugin.name === 'vite-plugin-svelte-kit') { return undefined; } return plugin; };

Thanks, the way you've got it right now is fine I think, and hopefully this is temporary anyway, until we can figure out what's causing the app to load instead of storybook in 7.0.

code/lib/builder-vite/src/vite-config.ts

IanVS · 2022-10-04T13:36:45Z

scripts/sandbox.ts

+        name: 'storybook:allow-template-stories',
+        config(config) {
+          if (config?.server?.fs?.allow) {
+            config.server.fs.allow.push('template-stories');


I think you could also just set server.fs.allow directly in the config, without needing to use a plugin here. That might even be a good test that such user-configured settings are properly honored, and that .storybook is being added correctly by the other plugin.

yeah I tried that but you actually can't because that won't pick up on other plugin's configs.
So if I set it directly on the config config?.server?.fs?.allow will be falsy, while doing it through this plugin logic config?.server?.fs?.allow will actually include vite-plugin-svelte-kit's allow list.

I meant without the check, you could just set the allow value directly, and then the rest of everything should still work. vite-plugin-svelte-kit should extend the user's config and the other plugin you created should as well, right? If we can't set custom allow paths in viteFinal, we're in trouble.

Hmm I don't think that I understand what you're saying, so bear with me.

My (verbose explanation of my) logic is:

If allow is empty it will default to allow the whole workspace root, so if we add to an empty allow we're effectively denying the rest, which might break stuff for the user.

So we have to check if allow is non-empty, before adding template-stories or other entries anywhere.

checking the content in allow directly when building the config is inadequate because some plugins might add entries to allow, and they won't be checkable at "build config"-time.

but we can check the content in allow by adding our own plugin, and from it read the config, and possibly add the entires to allow.

Which part of the above sounds wrong to you?

I think the plugin is probably the safer approach because it can check whether the user has set server.fs.allow themselves or not. If we always pass server.fs.allow: ['.storybook'] then we enable that feature which will may break the user's project if they have not added the directories their project uses to the config.

In this sandbox, we are the user. I'm not talking about setting it in all cases, just in the example sandbox, which is used for testing.

oh yeah, that sounds like the better approach then

Ah right, gotcha.
If we always set it in sandboxes then we might as well set it to '.' because we'd need to allow more than just 'template-stories'. I'm down with that.
Alternatively 'template-stories' + 'src' or similar.

I've allowed 'src', 'template-stories', 'node_modules' which works, but only if sources are always in src, but at least they were in the react-vite/default-ts sandbox so I think it's safe to assume.
You're right that this also serves as a good way to test that we're adding .storybook correctly.

…-544-sveltekit

…s repo

…-544-sveltekit

IanVS

This looks good to me, but I see CI failures like:

Unable to find the Storybook folder in "/tmp/storybook/sandbox/svelte-kit-skeleton-js/.storybook". Are you sure it exists? Or maybe this folder uses a custom Storybook config directory?

Is that because the new repros have not yet been published?

IanVS · 2022-10-05T11:51:30Z

code/frameworks/svelte-vite/src/preset.ts

@@ -24,7 +24,7 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets
 const removeSvelteKitPlugin = (plugins: PluginOption[]) => {
  plugins.forEach((plugin, index) => {
    if (plugin && 'name' in plugin && plugin.name === 'vite-plugin-svelte-kit') {
-      // eslint-disable-next-line no-param-reassign -- we explicitly want to mutate the array as stated in Vite docs
+      // eslint-disable-next-line no-param-reassign -- we explicitly want to mutate the array as stated here: https://vitejs.dev/guide/api-plugin.html#config


Thanks, the way you've got it right now is fine I think, and hopefully this is temporary anyway, until we can figure out what's causing the app to load instead of storybook in 7.0.

JReinhold · 2022-10-05T15:00:04Z

Is that because the new repros have not yet been published?

Yes I think that's probably it, they should work once we merge, or alternatively I can manually publish them - but I'll hold off until Node 16+ is ready in the repo.

IanVS · 2022-10-05T15:10:03Z

code/frameworks/angular/src/server/framework-preset-angular-ivy.ts

@@ -58,7 +58,7 @@ export const webpack = async (webpackConfig: Configuration, options: PresetOptio
    return webpackConfig;
  }

-  if(angularOptions.enableNgcc !== false) {
+  if (angularOptions.enableNgcc !== false) {


This seems unrelated to this PR, but I'm guessing it'll fall back out when next is updated and merged in.

benmccann · 2022-10-05T15:24:56Z

@JReinhold just FYI, it looks like there's a merge conflict

IanVS · 2022-10-12T15:39:38Z

Now that #19406 is merged, I guess this can be updated to leave the sveltekit plugin in the config, right?

…-544-sveltekit

JReinhold · 2022-10-13T18:35:52Z

I've disabled the SvelteKit repro for now because that was the only thing blocking this merge (waiting until we upgrade to node 16).

So this should be ready to merge now.

@IanVS I'm sorry for the timing, I should have thought of this sooner, before the Vite announcement.

IanVS · 2022-10-13T19:08:47Z

This looks good, but as far as I know, build-storybook will still be broken when using svelteKit (see #19280 (comment)).

Maybe we can merge this in, but not advertise sveltekit too hard until we can take care of that part.

JReinhold · 2022-10-13T19:23:11Z

Right, that looks gnarly.
It really seems like were struggling to incorporate a metaframework that (rightfully) wants to be it's own server and builder and isn't really built for others to hook into that (because why would it).

This will probably not be the last time facing these issues (hello SolidStart, Astro, Qwik City and Friends), so I wonder if we could think about this in a broader way..

But let's get this in and fix building the Storybook later.

IanVS · 2022-10-13T19:40:51Z

Awesome, thanks for slogging through and getting this in, @JReinhold!

shilman · 2022-10-15T02:45:31Z

@JReinhold FWIW I would call this a maintenance PR because it touches user-facing code in the Vite builder, and in a significant way.

JReinhold added 5 commits September 30, 2022 10:22

add sveltekit template

70f36b4

add actual repro configs using create-svelte-with-args

7d4c117

allow root dir in vite-builder

ceaf69b

remove sveltepreprocess from svelte templates

41091a6

remove vite-plugin-svelte-kit in config

f205c1c

JReinhold commented Oct 3, 2022

View reviewed changes

code/lib/builder-vite/src/vite-config.ts Outdated Show resolved Hide resolved

JReinhold commented Oct 3, 2022

View reviewed changes

code/lib/builder-vite/src/vite-server.ts Show resolved Hide resolved

JReinhold commented Oct 3, 2022

View reviewed changes

JReinhold self-assigned this Oct 3, 2022

JReinhold mentioned this pull request Oct 3, 2022

[7.0] Incompatible with sveltekit #19280

Closed

JReinhold added 2 commits October 4, 2022 01:45

Merge branch 'next' of github.com:storybookjs/storybook into jeppe/sb…

314d985

…-544-sveltekit

only allow storybook and template dirs

6056660

IanVS reviewed Oct 4, 2022

View reviewed changes

allow template-stories only when sandboxing

8d38821

IanVS reviewed Oct 4, 2022

View reviewed changes

JReinhold added 5 commits October 4, 2022 15:46

publish repros via gh actions

0a7f052

rename storybook vite plugin

bb3583a

always allow directories in sandboxes

22ff5e7

remove generate repros workflow

ea50752

Merge branch 'next' of github.com:storybookjs/storybook into jeppe/sb…

5a53bca

…-544-sveltekit

JReinhold marked this pull request as ready for review October 5, 2022 09:53

JReinhold added 3 commits October 5, 2022 12:14

add create-svelte-with-args to list of verdaccio packages outside thi…

fef7850

…s repo

Merge branch 'next' of github.com:storybookjs/storybook into jeppe/sb…

ab91bc8

…-544-sveltekit

Fix CircleCI parallelism

c2ccb55

JReinhold added build Internal-facing build tooling & test updates svelte labels Oct 5, 2022

IanVS reviewed Oct 5, 2022

View reviewed changes

IanVS approved these changes Oct 5, 2022

View reviewed changes

shilman changed the title ~~SvelteKit support for 7.0~~ Add SvelteKit sandbox testing Oct 6, 2022

JReinhold mentioned this pull request Oct 8, 2022

Ensure consistent server route handling by always starting managerBuilder before previewBuilder #19406

Merged

benmccann mentioned this pull request Oct 9, 2022

Storybook support sveltejs/kit#5397

Closed

JReinhold added 2 commits October 13, 2022 20:30

Merge branch 'next' of github.com:storybookjs/storybook into jeppe/sb…

5deb296

…-544-sveltekit

dont remove sveltekit plugins, disable sveltekit repro

7ecab8d

cleanup

26a2614

ensure no unused

da547d4

JReinhold merged commit 5e87fd3 into next Oct 13, 2022

JReinhold deleted the jeppe/sb-544-sveltekit branch October 13, 2022 19:35

IanVS changed the title ~~Add SvelteKit sandbox testing~~ Vite: Add partial SvelteKit support Oct 13, 2022

IanVS added the builder-vite label Oct 13, 2022

shilman added maintenance User-facing maintenance tasks and removed build Internal-facing build tooling & test updates labels Oct 15, 2022

JReinhold mentioned this pull request Nov 30, 2022

SvelteKit: Create framework #20039

Merged

2 tasks

		@@ -24,8 +25,23 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets

		plugins.push(svelteDocgen(config));

		removeSvelteKitPlugin(plugins);

Vite: Add partial SvelteKit support #19338

Vite: Add partial SvelteKit support #19338

Conversation

JReinhold commented Oct 3, 2022 • edited

TODO

What I did

How to test

Out of scope

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanVS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JReinhold commented Oct 5, 2022

Choose a reason for hiding this comment

benmccann commented Oct 5, 2022

IanVS commented Oct 12, 2022

JReinhold commented Oct 13, 2022 • edited

IanVS commented Oct 13, 2022

JReinhold commented Oct 13, 2022

IanVS commented Oct 13, 2022

shilman commented Oct 15, 2022

JReinhold commented Oct 3, 2022 •

edited

JReinhold commented Oct 13, 2022 •

edited