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 16 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
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ jobs:
executor:
class: medium+
name: sb_node_14_browsers
parallelism: 11
parallelism: 13
steps:
- git-shallow-clone/checkout_advanced:
clone_options: '--depth 1 --verbose'
Expand Down
7 changes: 3 additions & 4 deletions .github/workflows/generate-repros-next.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ on:
- cron: '2 2 */1 * *'
workflow_dispatch:
# To remove when the branch will be merged
push:
branches:
- vite-frameworks-xyz
- norbert/sb-524-webpack5react18ts
# push:
# branches:
# - jeppe/sb-544-sveltekit

jobs:
generate:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export const webpack = async (webpackConfig: Configuration, options: PresetOptio
return webpackConfig;
}

if(angularOptions.enableNgcc !== false) {
if (angularOptions.enableNgcc !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

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

runNgcc();
}

Expand Down
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 { hasPlugin } from './utils';
import { svelteDocgen } from './plugins/svelte-docgen';

Expand All @@ -20,8 +21,23 @@ export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets
// Add docgen plugin
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-storybook-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 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 @@ -126,6 +126,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 @@ -180,6 +206,7 @@ export default {
...vue2ViteTemplates,
...vue3ViteTemplates,
...svelteViteTemplates,
...svelteKitTemplates,
...litViteTemplates,
...vueCliTemplates,
// FIXME: missing documentation.json
Expand Down
5 changes: 4 additions & 1 deletion code/lib/telemetry/src/storybook-metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,10 @@ describe('await computeStorybookMetadata', () => {
},
});

expect(angularResult.framework).toEqual({ name: 'angular', options: { enableIvy: true, enableNgcc: true } });
expect(angularResult.framework).toEqual({
name: 'angular',
options: { enableIvy: true, enableNgcc: true },
});
});

test('should separate storybook packages and addons', async () => {
Expand Down
23 changes: 16 additions & 7 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,15 +246,23 @@ 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 source directories to complement any existing allow patterns
(".storybook" is already being allowed by builder-vite)
*/
function setSandboxViteFinal(mainConfig: ConfigFile) {
const viteFinalCode = `
(config) => ({
...config,
optimizeDeps: {
...config.optimizeDeps,
force: true,
optimizeDeps: { ...config.optimizeDeps, force: true },
server: {
...config.server,
fs: {
...config.server?.fs,
allow: ['src', 'template-stories', 'node_modules', ...(config.server?.fs?.allow || [])],
},
},
})`;
mainConfig.setFieldNode(
Expand Down Expand Up @@ -467,7 +476,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
4 changes: 4 additions & 0 deletions scripts/verdaccio.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ packages:
access: $all
publish: $all
proxy: npmjs
'@storybook/create-svelte-with-args':
access: $all
publish: $all
proxy: npmjs
'@storybook/react-docgen-typescript-plugin':
access: $all
publish: $all
Expand Down