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

Vite: Add partial SvelteKit support #19338

Merged
merged 20 commits into from
Oct 13, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions code/frameworks/svelte-vite/src/preset.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { StorybookConfig } from '@storybook/builder-vite';
import { PluginOption } from 'vite';
import { svelteDocgen } from './plugins/svelte-docgen';

export const addons: StorybookConfig['addons'] = ['@storybook/svelte'];
Expand All @@ -12,8 +13,23 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets

plugins.push(svelteDocgen(config));

removeSvelteKitPlugin(plugins);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


return {
...config,
plugins,
};
};

const removeSvelteKitPlugin = (plugins: PluginOption[]) => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 here: https://vitejs.dev/guide/api-plugin.html#config
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

plugins[index] = undefined;
}
if (Array.isArray(plugin)) {
// recursive, Vite plugins can be nested
removeSvelteKitPlugin(plugin);
}
});
};
13 changes: 13 additions & 0 deletions code/lib/builder-vite/src/vite-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,19 @@ export async function pluginConfig(options: ExtendedOptions) {
mdxPlugin(options),
injectExportOrderPlugin,
stripStoryHMRBoundary(),
{
name: 'storybook:allow-root-dir',
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
enforce: 'post',
config(config) {
// if there is NO allow list then Vite allows anything in the root directory
// if there is an allow list then Vite only allows anything in the listed directories
// add storybook specific directories only if there's an allow list so that we don't end up
// disallowing the root unless root is already disallowed
if (config?.server?.fs?.allow) {
config.server.fs.allow.push('.storybook');
}
},
},
] as PluginOption[];

// We need the react plugin here to support MDX in non-react projects.
Expand Down
29 changes: 15 additions & 14 deletions code/lib/builder-vite/src/vite-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,24 @@ import { getOptimizeDeps } from './optimizeDeps';
export async function createViteServer(options: ExtendedOptions, devServer: Server) {
const { presets } = options;

const config = await commonConfig(options, 'development');
const commonCfg = await commonConfig(options, 'development');

// Set up dev server
config.server = {
middlewareMode: true,
hmr: {
port: options.port,
server: devServer,
},
fs: {
strict: true,
const config = {
...commonCfg,
// Set up dev server
server: {
middlewareMode: true,
hmr: {
port: options.port,
server: devServer,
},
fs: {
strict: true,
},
},
appType: 'custom' as const,
optimizeDeps: await getOptimizeDeps(commonCfg, options),
};
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
config.appType = 'custom';

// TODO: find a way to avoid having to do this in a separate step.
config.optimizeDeps = await getOptimizeDeps(config, options);

const finalConfig = await presets.apply('viteFinal', config, options);
return createServer(finalConfig);
Expand Down
30 changes: 0 additions & 30 deletions code/lib/cli/src/generators/SVELTE/index.ts
Original file line number Diff line number Diff line change
@@ -1,43 +1,13 @@
import fse from 'fs-extra';
import { logger } from '@storybook/node-logger';

import { CoreBuilder } from '../../project_types';
import { baseGenerator } from '../baseGenerator';
import { Generator } from '../types';

const generator: Generator = async (packageManager, npmOptions, options) => {
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")()%%' },
};
}
}

Comment on lines -9 to -34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Does this need any MIGRATION.md notes? @JReinhold

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

const extraPackages = options.builder === CoreBuilder.Webpack5 ? ['svelte', 'svelte-loader'] : [];

await baseGenerator(packageManager, npmOptions, options, 'svelte', {
extraPackages,
extensions: ['js', 'jsx', 'ts', 'tsx', 'svelte'],
extraMain,
commonJs: true,
});
};
Expand Down
27 changes: 27 additions & 0 deletions code/lib/cli/src/repro-templates.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,32 @@ const svelteViteTemplates = {
// }
};

// SvelteKit only supports Node.js >16.x, so before generating these repros you need to switch to that version
const svelteKitTemplates = {
'svelte-kit/skeleton-js': {
name: 'Svelte Kit (JS)',
script:
'yarn create svelte-with-args --name=svelte-kit/skeleton-js --directory=. --template=skeleton --types=null --no-prettier --no-eslint --no-playwright',
cadence: ['ci', 'daily', 'weekly'],
expected: {
framework: '@storybook/svelte-vite',
renderer: '@storybook/svelte',
builder: '@storybook/builder-vite',
},
},
'svelte-kit/skeleton-ts': {
name: 'Svelte Kit (TS)',
script:
'yarn create svelte-with-args --name=svelte-kit/skeleton-ts --directory=. --template=skeleton --types=typescript --no-prettier --no-eslint --no-playwright',
cadence: ['ci', 'daily', 'weekly'],
expected: {
framework: '@storybook/svelte-vite',
renderer: '@storybook/svelte',
builder: '@storybook/builder-vite',
},
},
};

const litViteTemplates = {
'lit-vite/default-js': {
name: 'Lit Vite (JS)',
Expand Down Expand Up @@ -164,6 +190,7 @@ export default {
...vue2ViteTemplates,
...vue3ViteTemplates,
...svelteViteTemplates,
...svelteKitTemplates,
...litViteTemplates,
...vueCliTemplates,
// FIXME: missing documentation.json
Expand Down
24 changes: 20 additions & 4 deletions scripts/sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import prompts from 'prompts';
import type { AbortController } from 'node-abort-controller';
import command from 'execa';
import dedent from 'ts-dedent';

import { createOptions, getOptionsOrPrompt, OptionValues } from './utils/options';
import { executeCLIStep } from './utils/cli-step';
Expand Down Expand Up @@ -245,16 +246,31 @@ function addEsbuildLoaderToStories(mainConfig: ConfigFile) {
);
}

// Recompile optimized deps on each startup, so you can change @storybook/* packages and not
// have to clear caches.
function forceViteRebuilds(mainConfig: ConfigFile) {
/*
Recompile optimized deps on each startup, so you can change @storybook/* packages and not
have to clear caches.
And allow "template-stories" directory if necessary

*/
function setSandboxViteFinal(mainConfig: ConfigFile) {
const viteFinalCode = `
(config) => ({
...config,
optimizeDeps: {
...config.optimizeDeps,
force: true,
},
plugins: [
...config.plugins,
{
name: 'storybook:allow-template-stories',
config(config) {
if (config?.server?.fs?.allow) {
config.server.fs.allow.push('template-stories');
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

My (verbose explanation of my) logic is:

  1. 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.
  2. So we have to check if allow is non-empty, before adding template-stories or other entries anywhere.
  3. 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.
  4. 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, that sounds like the better approach then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
},
},
],
})`;
mainConfig.setFieldNode(
['viteFinal'],
Expand Down Expand Up @@ -467,7 +483,7 @@ export async function sandbox(optionValues: OptionValues<typeof options>) {
// Add some extra settings (see above for what these do)
mainConfig.setFieldValue(['core', 'disableTelemetry'], true);
if (builder === '@storybook/builder-webpack5') addEsbuildLoaderToStories(mainConfig);
if (builder === '@storybook/builder-vite') forceViteRebuilds(mainConfig);
if (builder === '@storybook/builder-vite') setSandboxViteFinal(mainConfig);

await writeConfig(mainConfig);

Expand Down