[7.0] Incompatible with sveltekit

IanVS commented 2 years ago

Describe the bug A standard bootstrap of a sveltekit + storybook project fails to start.

To Reproduce

yarn create svelte
follow the prompts, choose skeleton for the template and no for everything else
yarn
npx sb@future init
yarn storybook

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main defined in /Users/ianvs/code/experiments/sveltekit-generator/node_modules/@sveltejs/adapter-auto/package.json

It seems like this happens because of a require call here: https://github.com/storybookjs/storybook/blob/b42f0edea70f4f97d1d64aa86945f3beeff2a7b7/code/lib/core-common/src/utils/interpret-require.ts#L24

But sveltekit is ESM-only. I tried changing the generated .storybook/main.cjs file into esm, but that still didn't help.

System N/A

Additional context I've requested a way to specify all sveltekit options via command line flags, so that we can create a repro template for sveltekit and add it to our test suite: https://github.com/sveltejs/kit/issues/7064

IanVS commented 2 years ago

Bummer, my issue in sveltekit got closed already. This is maybe a downside of relying on community-generators, @ndelangen. What should we do when they don't expose command-line flags? Their suggestion is to write a script that uses their node api, but that seems to go against your philosophy of not creating any custom scripts ourselves.

cc @benmccann just for your awareness of this issue, and our difficulty in adding sveltekit to our CI process.

JReinhold commented 2 years ago

I'm actually working on creating a repro template for SvelteKit and ran into the exact same issue as you did - see https://github.com/sveltejs/kit/pull/6982

The plan I've agreed upon with @ndelangen is that we'll publish a small package that is just a wrapper around the create-svelte package so we can use the same methodology as we use with all our other repro templates.

that seems to go against your philosophy of not creating any custom scripts ourselves.

I agree that it's not ideal, but since it's such a simple wrapper that just passes arguments along I don't think it's a big deal. The biggest issue could be that we have to keep create-svelte up-to-date in that package which we usually don't have to worry about when using npx. We could just gitignore the yarn.lock file to ensure we always use the latest create-svelte version, but that seem a bit hazardous so I'm hoping for better suggestions.

You can follow along with the progress at https://github.com/storybookjs/create-svelte-with-args - but as you can see I haven't gotten very far. 😅 It is my main focus for now though, so should have something within the week.

JReinhold commented 2 years ago

Progress update from me.

I managed to release create-svelte-with-args and integrate it into our new repro template setup in the https://github.com/storybookjs/storybook/tree/jeppe/sb-544-sveltekit branch.

I run into the same CJS issue as you @IanVS, but I'm able to bypass it by commenting out the require call in the .storybook/main.cjs file:

// repros/svelte-kit/skeleton-js/after-storybook/.storybook/main.cjs
const path = require('path');
module.exports = {
  stories: ['../src/**/*.stories.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx|svelte)'],
  addons: [
    '@storybook/addon-links',
    '@storybook/addon-essentials',
    '@storybook/addon-interactions',
  ],
  framework: {
    name: '@storybook/svelte-vite',
    options: {},
  },
  svelteOptions: {
    // "preprocess": require("../svelte.config.js").preprocess
  },
};

We need to make this work of course, but one problem at a time.

The weird think I'm experiencing now is when running the storybook script with yarn storybook (alias for npx storybook dev -p 6006) it just opens the regular SvelteKit app, no Storybook UI in sight. But the terminal output looks fine:

@storybook/cli v7.0.0-alpha.34

info => Loading presets
info => Loading presets
info => Loading presets
info => Starting manager..
╭──────────────────────────────────────────────────────╮
│                                                      │
│   Storybook 7.0.0-alpha.34 for svelte-vite started   │
│   905 ms for manager and 894 ms for preview          │
│                                                      │
│    Local:            http://localhost:6006/          │
│    On your network:  http://192.168.1.189:6006/      │
│                                                      │
╰──────────────────────────────────────────────────────╯

AND http://localhost:6006/project.json actually returns a valid JSON file, the only suspicious thing being that it lists "webpack5" as the builder.

I'll keep digging into this and figure out what's going on. My hunch is that the vite-builder somehow picks up the wrong application.

benmccann commented 2 years ago

I wouldn't worry too much about making svelteOptions.preprocess work. I would highly discourage use of that option and would not mind removing it. builder-vite will now read the svelte.config.js itself so we shouldn't make the user be responsible for it (https://github.com/storybookjs/builder-vite/pull/428).

IanVS commented 2 years ago

@JReinhold thanks for the update! As for storybook launching the app, it's something we've seen before, and it should have been fixed in https://github.com/storybookjs/builder-vite/pull/453. I'd start by taking a look there to see if that option isn't getting set correctly now for some reason. I can dig into this as well in a day or two.

JReinhold commented 2 years ago

The CJS require call

Yeah that makes sense @benmccann, So essentially what we want to do is change the template so the "preprocess": require("../svelte.config.js").preprocess isn't part of main.cjs when we do sb init - right?

The missing Storybook problem

Thanks for the tip about appType @IanVS, but it wasn't it. I've figured out that the issue is caused by Storybook's Vite server merging ´builder-vite´'s own config with the "user's" (ie. the config in the sandbox) Vite config, which just adds the SvelteKit plugins to the config. I think the middlewares added by vite-plugin-svelte-kit gets first priority over everything else in the server, intercepting requests.

Commeting out the SvelteKit plugin in the sandbox's vite.config.js will make Storybook manager appear fine but all stories errors out with bad formatting, presumably because it doesn't compile Svelte now that SvelteKit's Vite plugin isn't added.

However, if we remove the plugin specific to SvelteKit - vite-plugin-svelte-kit - but keep all the default svelte plugins plugins, Storybook actually works with all stories functioning. Here's how I achieved this in viteFinal in main.cjs:

  viteFinal: (config) => {
+   const sveltePluginsWithoutKit = config.plugins[0].filter(
+     (plugin) => plugin.name !== 'vite-plugin-svelte-kit',
+   );

    const finalConfig = {
      ...config,
      optimizeDeps: {
        ...config.optimizeDeps,
        force: true,
      },
    };
+   finalConfig.plugins[0] = sveltePluginsWithoutKit;
    return finalConfig;
  },

I'm pretty sure this is not the right path forward, but it pinpoints the problem at least. I'm wondering if we want to somehow load vite-plugin-svelte-kit, or if we don't want that. I'd assume that it's mostly about routing and pages which doesn't make much sense within Storybook anyway, but it might also add stuff like the global stores and env that might break certain Components if they don't exist? What's your thoughts on this @benmccann ?

As a side note I assume that the reason this works in a barebone Svelte Vite app is because those Vite configs have all the necessary Svelte Vite plugins added while SvelteKit has both Svelte + SvelteKit, and thus by removing the SvelteKit plugin we're back to a barebone Svelte Vite app.

ndelangen commented 2 years ago

@JReinhold Let me know if you want to discuss this in a tuple or zoom or something 👍

IanVS commented 2 years ago

I did some poking around, and it seems to be the configureServer hook that sveltekit adds which is causing the app to be served instead of storybook. @benmccann do you know of any way we can prevent this within storybook? I think we want to keep the sveltekit plugin, rather than filtering it out, right?

JReinhold commented 2 years ago

@IanVS you're right actually, I figured out an easy way to remove that. It also requires allowing our root directory because otherwise preview.js is denied. ( we could probably fine tune this) I got a very crude PoC to work by changing the viteFinal function in code/frameworks/svelte-vite/src/preset.ts to:

export const viteFinal: StorybookConfig['viteFinal'] = async (config, { presets }) => {
  const { plugins = [] } = config;

  plugins.push(svelteDocgen(config));

  // plugins[0][2] is the vite-plugin-svelte-kit 
  delete plugins[0][2].configureServer;
  // maybe also delete plugins[0][2].configurePreviewServer; ?

  return {
    ...config,
    plugins,
    server: {
      ...config.server,
      fs: {
        ...config.server.fs,
        allow: ['.'],
      },
    },
  };
};

I'll investigate further later, just wanted to keep you up-to-date

IanVS commented 2 years ago

It looks like we lost this custom plugin in the move to 7.0: https://github.com/storybookjs/builder-vite/pull/427.

Though, I'm not sure it will work in this case, since the sveltekit plugin is using a config hook to add the allow list, so it won't show up in the config in viteFinal.

I think maybe we need to check for the presence of the sveltekit plugin, and if found, add .storybook to the allow list and delete the configureServer. If we end up having to do too much more config, I wonder if eventually we'll want sveltekit to have its own storybook framework, rather than adding a bunch of conditional logic into the svelte-vite framework. As far as I know, that was kind of the point of the framework refactor. And in fact, svelte themselves could publish a storybook framework that's tailored to meet their needs.

benmccann commented 2 years ago

I'm surprised configureServer is causing issues in 7.0, but not 6.0. Here's an example 6.0 project.

But anyway, I'm fairly skeptical that people should be writing SvelteKit-specific components. I almost think it's better that it's broken when you try to use SvelteKit functionality as it will force you to better structure your components. If you're writing Svelte components I'd generally expect them to work in any Svelte project and not just SvelteKit projects.

I could see it being a bit annoying to people that they can't do import { browser, dev, prerendering } from '$app/environment';. But this is also a problem we have when users run svelte-kit package to bundle their libraries for deployment (e.g. https://github.com/sveltejs/kit/issues/1950 and https://github.com/sveltejs/kit/issues/5879), so I think we need a more general solution.

For now, I would say as long as Svelte libraries work, we're probably best off ignoring SvelteKit issues.

benmccann commented 2 years ago

Adding .storybook to the fs allow list sounds like something that should be handled in the Storybook Vite builder regardless of whether you're using SvelteKit as it doesn't have anything to do with SvelteKit.

IanVS commented 2 years ago

@benmccann thanks, I agree we need to handle the fs.allow, likely by porting your plugin from the PR I linked into 7.0 (not sure how it got lost).

And to be clear on your other point, are you suggesting that we should filter out the vite-plugin-svelte-kit plugin entirely?

benmccann commented 2 years ago

I don't quite understand yet why vite-plugin-svelte-kit would break things with 7.0, but not 6.0. Is 7.0 starting a Vite dev server where 6.0 was not? I think it's probably fine to filter out vite-plugin-svelte-kit, but I'm not sure it's the only plugin you'll encounter that has implemented configureServer. Will the others using that hook break too and you instead need some more general implementation that deletes that hook off all plugins?

IanVS commented 2 years ago

I don't quite understand yet why vite-plugin-svelte-kit would break things with 7.0, but not 6.0. Is 7.0 starting a Vite dev server where 6.0 was not?

I'm pretty stumped by this as well. Not that much changed in the vite builder, mostly just that we read the vite.config.js automatically, and I found a few other minor differences as well, but no matter what I do I can't get 6.5 to reproduce the behavior of 7.0 or vice-versa. Very weird...

The only thing I can think of is that in 7.0, the storybook ui itself is built using esbuild, whereas before it was bundled with webpack. It's a stretch, but maybe that's got something to do with it?

Adding /iframe.html to the url does seem to at least try to load stories, though it fails for a reason I don't understand so far...

JReinhold commented 2 years ago

I think it's probably fine to filter out vite-plugin-svelte-kit, but I'm not sure it's the only plugin you'll encounter that has implemented configureServer. Will the others using that hook break too and you instead need some more general implementation that deletes that hook off all plugins? - @benmccann

vite-plugin-svelte actually also has a configureServer entry which sets up some watchers (https://github.com/sveltejs/vite-plugin-svelte/blob/main/packages/vite-plugin-svelte/src/index.ts#L98-L102), and I don't think we want to exclude those, so that's maybe a reason to not globally remove configureServer but just remove vite-plugin-svelte-kit instead?

JReinhold commented 2 years ago

I've done an initial PR based on our discussions at https://github.com/storybookjs/storybook/pull/19338

IanVS commented 2 years ago

Ideally we wouldn't have to remove anything. We don't need to with 6.5, and I'm trying to figure out what's different about 7.0.

JReinhold commented 2 years ago

I think I've figured out why SvelteKit breaks in 7.0 but not in 6.5.

SvelteKit in 6.5 vs 7.0

As you say @IanVS, builder-vite automatically loads the user's vite.config.js in 7.0 but not in 6.5. And in 6.5 we manually load the necessary Svelte Vite plugins, which doesn't include vite-plugin-svelte-kit. This means that in 6.5 we actually never load vite-plugin-svelte-kit, even in a SvelteKit app. But because of the auto-loading in 7.0 (which I think is a good thing) we get it there, and then it causes us troubles.

My steps were:

New project with npx create-svelte@latest
npx sb init
Remove faulty svelteOptions from main.cjs (see below)
Check that everything works, both the app and Storybook.
Add console.dir(finalConfig, { depth: 10 }) just before createServer() in node_modules/@storybook/builder-vite/dist/vite-server.js which I think is the last possible place to log it.
See that the output doesn't include the vite-plugin-svelte-kit plugin.
Add a random plugin to my vite.config.js like:

const config: UserConfig = {
    plugins: [sveltekit(), { name: "jeppes-custom-vite-plugin" }],
};

Run everything again and see that it too isn't part of the log.

So I think that we should continue with our current solution for now because it works as it used to, and if we want more of the SvelteKit goodness like importing globals and envs, the SvelteKit team would have to separate that functionality from the route handling functionality for us to be able to extract that. (not saying I expect this to happen, just saying it's the only option I see in that regard)

For reference here's the log from the finalConfig:

```js { configFile: false, root: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183', cacheDir: 'node_modules/.vite-storybook', envPrefix: [ 'VITE_', 'STORYBOOK_' ], define: { 'process.env.XSTORYBOOK_EXAMPLE_APP': '""', 'import.meta.env.STORYBOOK': '"true"', 'import.meta.env': '{"STORYBOOK":"\\"true\\""}' }, resolve: {}, plugins: [ { name: 'storybook-vite-code-generator-plugin', enforce: 'pre', configureServer: [Function: configureServer], config: [Function: config], configResolved: [Function: configResolved], resolveId: [Function: resolveId], load: [AsyncFunction: load], transformIndexHtml: [AsyncFunction: transformIndexHtml] }, { name: 'storybook-vite-source-loader-plugin', enforce: 'pre', transform: [AsyncFunction: transform] }, { name: 'storybook-vite-mdx-plugin', enforce: 'pre', configResolved: [Function: configResolved], transform: [AsyncFunction: transform] }, { name: 'no-fouc', enforce: 'post', transformIndexHtml: [AsyncFunction: transformIndexHtml] }, { name: 'storybook-vite-inject-export-order-plugin', enforce: 'post', transform: [AsyncFunction: transform] }, [ { name: 'vite:react-babel', enforce: 'pre', config: [Function: config], configResolved: [Function: configResolved], transform: [AsyncFunction: transform] }, { name: 'vite:react-refresh', enforce: 'pre', config: [Function: config], resolveId: [Function: resolveId], load: [Function: load], transformIndexHtml: [Function: transformIndexHtml] }, { name: 'vite:react-jsx', enforce: 'pre', config: [Function: config], resolveId: [Function: resolveId], load: [Function: load] } ], { name: 'vite-plugin-storybook-allow', enforce: 'post', config: [Function: config] }, [ { name: 'vite-plugin-svelte', enforce: 'pre', api: { options: { hot: { injectCss: false, partialAccept: true }, compilerOptions: { css: false, dev: true, hydratable: true }, extensions: [ '.svelte' ], emitCss: true, preprocess: [ { defaultLanguages: { markup: 'html', style: 'css', script: 'javascript' }, markup: [AsyncFunction: markup], script: [AsyncFunction: script], style: [AsyncFunction: style] }, { style: [Function: style] } ], kit: { adapter: { name: '@sveltejs/adapter-auto', adapt: [Function: adapt] } }, configFile: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183/svelte.config.js', exclude: [ '**/*.story.svelte', '**/*.stories.svelte' ], root: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183', isBuild: false, isServe: true, isDebug: false, isProduction: false } }, config: [AsyncFunction: config], configResolved: [AsyncFunction: configResolved], buildStart: [AsyncFunction: buildStart], configureServer: [Function: configureServer], load: [Function: load], resolveId: [AsyncFunction: resolveId], transform: [AsyncFunction: transform], handleHotUpdate: [Function: handleHotUpdate] }, { name: 'vite-plugin-svelte:inspector', apply: 'serve', enforce: 'pre', configResolved: [Function: configResolved], resolveId: [AsyncFunction: resolveId], load: [AsyncFunction: load], transform: [Function: transform], transformIndexHtml: [Function: transformIndexHtml] } ], { name: 'vite-plugin-svelte-stories', enforce: 'pre', api: { options: { hot: false, compilerOptions: { css: false, dev: true, hydratable: true }, extensions: [ '.svelte' ], emitCss: true, preprocess: { defaultLanguages: { markup: 'html', style: 'css', script: 'javascript' }, markup: [AsyncFunction: markup], script: [AsyncFunction: script], style: [AsyncFunction: style] }, kit: { adapter: { name: '@sveltejs/adapter-auto', adapt: [Function: adapt] } }, configFile: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183/svelte.config.js', exclude: [], include: [ '**/*.story.svelte', '**/*.stories.svelte' ], root: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183', isBuild: false, isServe: true, isDebug: false, isProduction: false } }, config: [AsyncFunction: config], configResolved: [AsyncFunction: configResolved], buildStart: [AsyncFunction: buildStart], configureServer: [Function: configureServer], load: [Function: load], resolveId: [AsyncFunction: resolveId], transform: [AsyncFunction: transform], handleHotUpdate: [Function: handleHotUpdate] }, { name: 'svelte-docgen', transform: [AsyncFunction: transform] } ], server: { middlewareMode: true, hmr: { port: 6007, server: Server { maxHeaderSize: undefined, insecureHTTPParser: undefined, _events: [Object: null prototype] { request: [Function: app] { _events: [Object: null prototype] { mount: [Function: onmount] }, _eventsCount: 1, _maxListeners: undefined, setMaxListeners: [Function: setMaxListeners], getMaxListeners: [Function: getMaxListeners], emit: [Function: emit], addListener: [Function: addListener], on: [Function: addListener], prependListener: [Function: prependListener], once: [Function: once], prependOnceListener: [Function: prependOnceListener], removeListener: [Function: removeListener], off: [Function: removeListener], removeAllListeners: [Function: removeAllListeners], listeners: [Function: listeners], rawListeners: [Function: rawListeners], listenerCount: [Function: listenerCount], eventNames: [Function: eventNames], init: [Function: init], defaultConfiguration: [Function: defaultConfiguration], lazyrouter: [Function: lazyrouter], handle: [Function: handle], use: [Function: use], route: [Function: route], engine: [Function: engine], param: [Function: param], set: [Function: set], path: [Function: path], enabled: [Function: enabled], disabled: [Function: disabled], enable: [Function: enable], disable: [Function: disable], acl: [Function (anonymous)], bind: [Function (anonymous)], checkout: [Function (anonymous)], connect: [Function (anonymous)], copy: [Function (anonymous)], delete: [Function (anonymous)], get: [Function (anonymous)], head: [Function (anonymous)], link: [Function (anonymous)], lock: [Function (anonymous)], 'm-search': [Function (anonymous)], merge: [Function (anonymous)], mkactivity: [Function (anonymous)], mkcalendar: [Function (anonymous)], mkcol: [Function (anonymous)], move: [Function (anonymous)], notify: [Function (anonymous)], options: [Function (anonymous)], patch: [Function (anonymous)], post: [Function (anonymous)], propfind: [Function (anonymous)], proppatch: [Function (anonymous)], purge: [Function (anonymous)], put: [Function (anonymous)], rebind: [Function (anonymous)], report: [Function (anonymous)], search: [Function (anonymous)], source: [Function (anonymous)], subscribe: [Function (anonymous)], trace: [Function (anonymous)], unbind: [Function (anonymous)], unlink: [Function (anonymous)], unlock: [Function (anonymous)], unsubscribe: [Function (anonymous)], all: [Function: all], del: [Function (anonymous)], render: [Function: render], listen: [Function: listen], request: IncomingMessage { app: [Circular *1] }, response: ServerResponse { app: [Circular *1] }, cache: {}, engines: {}, settings: { 'x-powered-by': true, etag: 'weak', 'etag fn': [Function: generateETag], env: 'development', 'query parser': 'extended', 'query parser fn': [Function: parseExtendedQueryString], 'subdomain offset': 2, 'trust proxy': false, 'trust proxy fn': [Function: trustNone], view: [Function: View], views: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183/views', 'jsonp callback name': 'callback' }, locals: [Object: null prototype] { settings: { 'x-powered-by': true, etag: 'weak', 'etag fn': [Function: generateETag], env: 'development', 'query parser': 'extended', 'query parser fn': [Function: parseExtendedQueryString], 'subdomain offset': 2, 'trust proxy': false, 'trust proxy fn': [Function: trustNone], view: [Function: View], views: '/Users/jeppe/dev/temp/sveltekit-default-2.0.0-next.183/views', 'jsonp callback name': 'callback' } }, mountpath: '/', _router: [Function: router] { params: {}, _params: [], caseSensitive: false, mergeParams: undefined, strict: false, stack: [ Layer { handle: [Function: query], name: 'query', params: undefined, path: undefined, keys: [], regexp: /^\/?(?=\/|$)/i { fast_star: false, fast_slash: true }, route: undefined }, Layer { handle: [Function: expressInit], name: 'expressInit', params: undefined, path: undefined, keys: [], regexp: /^\/?(?=\/|$)/i { fast_star: false, fast_slash: true }, route: undefined }, Layer { handle: [Function: compression], name: 'compression', params: undefined, path: undefined, keys: [], regexp: /^\/?(?=\/|$)/i { fast_star: false, fast_slash: true }, route: undefined }, Layer { handle: [Function (anonymous)], name: '', params: undefined, path: undefined, keys: [], regexp: /^\/?(?=\/|$)/i { fast_star: false, fast_slash: true }, route: undefined }, Layer { handle: [Function: router] { params: {}, _params: [], caseSensitive: undefined, mergeParams: undefined, strict: undefined, stack: [ [Layer], [Layer], [Layer], [Layer] ] }, name: 'router', params: undefined, path: undefined, keys: [], regexp: /^\/?(?=\/|$)/i { fast_star: false, fast_slash: true }, route: undefined } ] } }, connection: [Function: connectionListener], upgrade: [Function (anonymous)] }, _eventsCount: 3, _maxListeners: undefined, _connections: 0, _handle: TCP { reading: false, onconnection: [Function: onconnection], [Symbol(owner_symbol)]: [Circular *2] }, _usingWorkers: false, _workers: [], _unref: false, allowHalfOpen: true, pauseOnConnect: false, noDelay: false, keepAlive: false, keepAliveInitialDelay: 0, httpAllowHalfOpen: false, timeout: 0, keepAliveTimeout: 5000, maxHeadersCount: null, maxRequestsPerSocket: 0, headersTimeout: 60000, requestTimeout: 0, _connectionKey: '6::::6007', [Symbol(IncomingMessage)]: [Function: IncomingMessage], [Symbol(ServerResponse)]: [Function: ServerResponse], [Symbol(kCapture)]: false, [Symbol(async_id_symbol)]: 48, [Symbol(kUniqueHeaders)]: null } }, fs: { strict: true } }, appType: 'custom', optimizeDeps: { entries: [ 'src/stories/Introduction.stories.mdx', 'src/stories/Button.stories.js', 'src/stories/Header.stories.js', 'src/stories/Page.stories.js' ], include: [ '@mdx-js/react', '@storybook/addon-docs > acorn-jsx', '@storybook/addon-docs', '@storybook/addons', '@storybook/channel-postmessage', '@storybook/channel-websocket', '@storybook/client-api', '@storybook/client-logger', '@storybook/core/client', '@storybook/csf', '@storybook/preview-web', '@storybook/react > acorn-jsx', '@storybook/svelte', 'acorn-jsx', 'acorn', 'airbnb-js-shims', 'ansi-to-html', 'color-convert', 'doctrine', 'estraverse', 'fast-deep-equal', 'global', 'isobject', 'jest-mock', 'loader-utils', 'lodash/cloneDeep', 'lodash/isFunction', 'lodash/isPlainObject', 'lodash/isString', 'lodash/mapKeys', 'lodash/mapValues', 'lodash/pick', 'lodash/pickBy', 'lodash/startCase', 'lodash/throttle', 'lodash/uniq', 'memoizerific', 'polished', 'prettier/parser-babel', 'prettier/parser-flow', 'prettier/parser-typescript', 'prop-types', 'qs', 'react-dom', 'react-dom/client', 'react-is', 'react', 'react/jsx-runtime', 'regenerator-runtime/runtime.js', 'slash', 'stable', 'store2', 'synchronous-promise', 'telejson', 'ts-dedent', 'unfetch', 'util-deprecate', 'uuid-browser/v4' ] } } ```

Breaking `svelteOptions`

As my PR for 7.0 removes the svelteOptions in main.cjs to fix the CJS vs ESM issue we actually need to do something similar for 6.5, because right now it doesn't work if you/the user don't remove that line manually. However I'm not sure we can actually do that in 6.5 because I don't think the options are auto-loaded by Vite/the Vite builder in 6.5 as they are in 7.0, so I'm afraid removing that will mean that we won't have the user's custom svelteOptions in their Storybook. I could be wrong here though, please correct me. I'm not sure what the correct way forward here is actually, other than telling SvelteKit users that they have to wait for 7.0 to land? I hoped there were a quick win to make 6.5 work though.

IanVS commented 2 years ago

@JReinhold even when including sveltekit plugin in 6.5, this issue didn't happen. Ben pointed to a 6.5 example sveltekit app with storybook in the issue, and it works correctly.

And yes, the vite builder does load svelteoptions itself in 6.5.

JReinhold commented 2 years ago

@IanVS you're right, I totally missed that. Then I'm all out of clues, I even tried to do a file-by-file comparison between vite-builder 6.5 and 7.0 and came up empty handed. I also tried to move the SvelteKit plugin loading further up the chain in 6.5 because I thought that maybe the order of the plugins had an effect, but it didn't.

IanVS commented 2 years ago

Yeah, I tried things like that, even removing all other plugins and making sure the rest of the vite config was identical. That's why the only thing I can think of is that it has something to do with the change moving the manager away from weback and to esbuild, though as far as I can tell, the dev server is unchanged and is still just an express server.

gbkwiatt commented 2 years ago

Just to make sure this issue here is related. I am trying to deploy sveltekit storybook with latest storybook alpha and alpha @storybook/svelte-vite as per default configuration. But upon web preview I get Expected your framework's preset to export arenderToDOMfield.

Is that what you are trying to sort out here or it's not related ?

IanVS commented 2 years ago

@gbkwiatt that looks like a different error. I was able to bootstrap a new sveltekit project, run npx sb@next init, and then tweak the .storybook/main.cjs file a little:

const {mergeConfig} = require('vite');
module.exports = {
  "stories": [
    "../src/**/*.stories.mdx",
    "../src/**/*.stories.@(js|jsx|ts|tsx|svelte)"
  ],
  "addons": [
    "@storybook/addon-links",
    "@storybook/addon-essentials",
    "@storybook/addon-interactions"
  ],
  "framework": {
    "name": "@storybook/svelte-vite",
    "options": {}
  },
  viteFinal(config) {
    return mergeConfig(config, {
      server: {
        fs: {
          allow: ['.storybook'],
        }
      }
    })
  }
}

and storybook started up successfully after that.

This workaround in the main.cjs will only be needed until https://github.com/storybookjs/storybook/pull/19338 is updated and released.

gbkwiatt commented 2 years ago

Weird, I've created latest basic sveltekit project and created some basic story, and when it tries to render it's just that

~~Also when I am using your config above I get config.server.fs.allow.push is not a function ERR! at config (/home/gbkwiatt/storybook/node_modules/@storybook/builder-vite/dist/vite-config.js:96:44)~~ Above fixed when changing allow: to array ['.storybook'], but other error still persists

IanVS commented 2 years ago

@gbkwiatt can you share the output of npx sb@next info? I wonder if you have an old storybook package hanging around somehow.

gbkwiatt commented 2 years ago

npx sb@next info

System:
    OS: Linux 5.10 Ubuntu 20.04.4 LTS (Focal Fossa)
    CPU: (16) x64 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz
  Binaries:
    Node: 18.1.0 - ~/.nvm/versions/node/v18.1.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.1.0/bin/yarn
    npm: 8.8.0 - ~/.nvm/versions/node/v18.1.0/bin/npm
  npmPackages:
    @storybook/addon-actions: next => 7.0.0-alpha.37 
    @storybook/addon-essentials: next => 7.0.0-alpha.37 
    @storybook/addon-interactions: next => 7.0.0-alpha.37 
    @storybook/addon-links: next => 7.0.0-alpha.37 
    @storybook/addon-svelte-csf: ^2.0.8 => 2.0.8 
    @storybook/builder-vite: ^0.2.4 => 0.2.4 
    @storybook/svelte: next => 7.0.0-alpha.37 
    @storybook/svelte-vite: ^7.0.0-alpha.37 => 7.0.0-alpha.37 
    @storybook/testing-library: ^0.0.13 => 0.0.13

IanVS commented 2 years ago

Ah, yep. @storybook/builder-vite should be the same alpha version as the rest.

gbkwiatt commented 2 years ago

Rookie mistake - I thought I've checked all deps. Clearly not ;) Thanks that works

@IanVS however I keep fighting with issue for a long while - when building storybook, it does not do complete build ( missing iframe.html) and it's the case still. I can't figure it out, build completes but files are missing and it does not work when deployed

IanVS commented 2 years ago

Are you deploying in a subdirectory? Maybe you're hitting https://github.com/storybookjs/builder-vite/issues/238? If you want to hop into the #vite channel in https://discord.gg/storybook, I'd be happy to help you troubleshoot.

IanVS commented 2 years ago

@benmccann I'm seeing this when trying to build:

The following Vite config options will be overridden by SvelteKit:
  - build.emptyOutDir
  - build.outDir
  - build.rollupOptions.input
  - root

Is there a way to prevent sveltekit from overriding those settings?

I guess maybe we'll need to create our own plugin with config hook that un-overrides them. 😆

Hmmm, but then it still fails because sveltekit looks for certain things in a specific spot...

ENOENT: no such file or directory, open '/Users/ianvs/code/experiments/sveltekit/.svelte-kit/output/client/_app/version.json'

JReinhold commented 2 years ago

In a perfect world the SvelteKit Vite plugin could be loaded with something like

...
plugins: [sveltekit({ server: false, builder: false })]
...

that would disable route handling as well as build outputs, etc. This could allow us to include the SvelteKit features without having to battle with it for who gets to be the "main" handler. \ (Although I can image some of the SvelteKit features would be impossible to include without Kit also being the main handler)

But I assume that we're a pretty niche use case and I can't imagine why anyone other than Storybook would benefit from this, so I can understand if the Svelte team is hesitant to support this kind of thing.

Alternatively, separating the SvelteKit features from from server/build handling into separate sub-vite-plugins so we could disable the necessary plugin manually. Basically the same as above but in a less official way.

shilman commented 2 years ago

Son of a gun!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-alpha.38 containing PR #19338 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

IanVS commented 2 years ago

https://github.com/storybookjs/storybook/issues/19280#issuecomment-1277810919 is still a problem (can't build).

benmccann commented 2 years ago

In a perfect world the SvelteKit Vite plugin could be loaded with something like ... plugins: [sveltekit({ server: false, builder: false })] ... that would disable route handling as well as build outputs, etc.

That seems almost equivalent to removing the SvelteKit plugins, so would be roughly the same as doing:

if (!STORYBOOK) {
  config.plugins.push(sveltekit())
}

This could allow us to include the SvelteKit features* without having to battle with it for who gets to be the "main" handler.

(Although I can image some of the SvelteKit features would be impossible to include without Kit also being the main handler)

Are there particular features you're thinking of? If you don't build the SvelteKit generated code such as the route handlers, I'm not sure you're left with much...

I think stripping out the SvelteKit plugin is probably a fine solution at least for now. I generally think that Svelte components should not be built containing SvelteKit-specific code. However, if folks have a use for it I'd be curious to hear about it as it's something we could reconsider given a compelling usecase.

Amerlander commented 2 years ago

I agree with benmccann. But I can give some examples for SvelteKit-specific code I used in my components and had to change to use with Storybook. It were only about the SvelteKit specific $app/environment browser variable and $app/stores page store:

I have a menu component which uses the page store:

import { page } from '$app/stores'
$: active = $page.url.pathname;

I haven't thought about a workaround yet, I just decided to not add this component to storybook for now.

And I had components which used the browser variable to prevent code from being ssr. Those components are already changed to not use browser anymore and I think that also improved the components. Also, I think the browser variable for example could be hard coded to be always true in storybook and then easily be used.

I would rather prefer to be able to build storybook than to have these sveltekit specific things available.

JReinhold commented 2 years ago

I'll do a follow up PR that removes the SvelteKit Vite plugin.

Are there particular features you're thinking of? If you don't build the SvelteKit generated code such as the route handlers, I'm not sure you're left with much...

I think it's mainly importing the SvelteKit specific modules. I agree most of them don't make sense in a deep component that you'd have in a Storybook story, eg. @sveltejs/kit/hooks doesn't make sense in a Storybook context. But I can see some of them make sense, eg.:

$app/environment - As @Amerlander mentions should probably always be browser in SB
Supporting any $lib/ imports - I'd guess is just a simple Vite alias.
$app/paths - components showing the current URL or a path to an asset or whatever
$app/stores - the page and the navigation stores could be useful for components that navigates.

But I think all of these should ideally be treated similar to props (or "args"), so users could "mock" them or define them as needed per story. Setting the current page for a given story, or making $app/navigation/goto call a Storybook action when called. I think all of this is out of scope for our SvelteKit support for now, but I think it would be the ideal DX for anyone or the community to build.

My hunch is that the biggest annoyance would be that we don't support $lib/ imports, I can see that being used quite often in any component.

shilman commented 2 years ago

Son of a gun!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-alpha.41 containing PR #19522 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

jacob-8 commented 2 years ago

When I init and run Storybook 7 with a new SvelteKit project, I get this error on Windows:

[vite] error while updating dependencies:
Error: ENOENT: no such file or directory, 
rename 'C:\dev\svelte-storybook7\node_modules\.cache\.vite-storybook\deps_temp' 
    -> 'C:\dev\svelte-storybook7\node_modules\.cache\.vite-storybook\deps'

The storybook shell will load but each page has no content.

To reproduce:

npm create svelte@latest
choose skeleton project, typescript
npm i
npx sb@next init (I'm running npm 8.17.0 so I agree to the prompt to run the npm7 migration on my project)
npm run storybook

IanVS commented 2 years ago

Unfortunately that's a vite error: https://github.com/vitejs/vite/issues/9986.

You may be able to get past it by removing the node_modules/.cache directory, and restarting storybook.

storybookjs / storybook