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 5 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,6 +1,7 @@
import path from 'path';
import fs from 'fs';
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 @@ -24,8 +25,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 in Vite docs
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -88,6 +88,19 @@ export async function pluginConfig(options: ExtendedOptions) {
mdxPlugin(options),
injectExportOrderPlugin,
stripStoryHMRBoundary(),
{
name: 'vite-plugin-storybook-allow-root-dir',
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 the root directory only if there's an allow list so that we don't end up
// disallowing the other directories unless root is already disallowed
if (config?.server?.fs?.allow) {
config.server.fs.allow.push('.');
JReinhold marked this conversation as resolved.
Show resolved Hide resolved
}
},
},
] 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 @@ -94,6 +94,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 vueCliTemplates = {
'vue-cli/default-js': {
name: 'Vue-CLI (Default JS)',
Expand Down Expand Up @@ -126,6 +152,7 @@ export default {
...reactViteTemplates,
...vue3ViteTemplates,
...svelteViteTemplates,
...svelteKitTemplates,
...vueCliTemplates,
// FIXME: missing documentation.json
// 'angular/latest': {
Expand Down
1 change: 1 addition & 0 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