storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.61k stars 9.31k forks source link

[Bug]: Svelte stories errors with `'target' is a required option` #19634

Closed JReinhold closed 2 years ago

JReinhold commented 2 years ago

Describe the bug

When creating a fresh Svelte project and adding Storybook with npx sb@next init, none of the example stories render correctly, instead throwing the error:

Error: 'target' is a required option
    at new SvelteComponentDev (http://localhost:6006/node_modules/.cache/.vite-storybook/deps/chunk-DSHMA543.js?v=29501131:1959:13)
    at new Header (http://localhost:6006/src/stories/Header.svelte:453:3)
    at createProxiedComponent (http://localhost:6006/node_modules/svelte-hmr/runtime/svelte-hooks.js?v=29501131:341:9)
    at new ProxyComponent (http://localhost:6006/node_modules/svelte-hmr/runtime/proxy.js?v=29501131:242:7)
    at new Proxy<Header> (http://localhost:6006/node_modules/svelte-hmr/runtime/proxy.js?v=29501131:349:11)
    at construct_svelte_component (http://localhost:6006/node_modules/.cache/.vite-storybook/deps/chunk-DSHMA543.js?v=29501131:756:10)
    at Array.K (http://localhost:6006/node_modules/.cache/.vite-storybook/deps/chunk-CVDXXG7Q.js?v=29501131:67:20)
    at Z (http://localhost:6006/node_modules/.cache/.vite-storybook/deps/chunk-CVDXXG7Q.js?v=29501131:165:39)
    at init (http://localhost:6006/node_modules/.cache/.vite-storybook/deps/chunk-DSHMA543.js?v=29501131:1751:35)
    at new L (http://localhost:6006/node_modules/.cache/.vite-storybook/deps/chunk-CVDXXG7Q.js?v=29501131:211:14)

Googling the error make it seems like it's a fairly common error related to modules, building or similar.

To Reproduce

standalone SvelteKit setup

npm create svelte@latest sb-issue-19634
// choose defaults
cd sb-issue-19634
npx sb@next init
npm run storybook

svelte-vite/default-js Sandbox

Within the SB repo:

yarn task --task sandbox --template svelte-vite/default-js
cd sandbox/svelte-vite-default-js/
yarn storybook
Open any story

System

System:
    OS: macOS 12.3
    CPU: (10) arm64 Apple M1 Pro
  Binaries:
    Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node
    Yarn: 3.2.4 - /opt/homebrew/bin/yarn
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.0/bin/npm
  Browsers:
    Chrome: 106.0.5249.119
    Firefox: 105.0.2
    Safari: 15.4
  npmPackages:
    @storybook/addon-essentials: ^7.0.0-alpha.40 => 7.0.0-alpha.43
    @storybook/addon-interactions: ^7.0.0-alpha.40 => 7.0.0-alpha.43
    @storybook/addon-links: ^7.0.0-alpha.40 => 7.0.0-alpha.43
    @storybook/jest: ^0.0.10 => 0.0.10
    @storybook/svelte: ^7.0.0-alpha.40 => 7.0.0-alpha.43
    @storybook/svelte-vite: ^7.0.0-alpha.40 => 7.0.0-alpha.43
    @storybook/test-runner: next => 0.10.0-next.1
    @storybook/testing-library: next => 0.0.14-next.0

Additional context

  1. This only errors in dev mode - builds work fine (which is also why we haven't caught it in our sandbox Chromatic setup)
  2. I've narrowed it down to an issue being introduced with v7.0.0-alpha.42. Most likely https://github.com/storybookjs/storybook/pull/19512 because the other PRs in that release doesn't touch Svelte nor Vite AFAIK. On the surface I can't see any changes in that PR that would cause this as it's mostly just types and tests. @kasperpeulen can you point to any runtime changes you made there that could cause the rendering/building to break?
  3. This is both an issue in SvelteKit setups and in our svelte-vite/default-js sandbox so I think we can rule out SvelteKit issues here.

I narrowed it down by creating a new Svelte project with Storybook and pinning all the versions to 7.0.0-alpha.41(without the ^) in package.json and ran npm install. This worked, but changing it to 7.0.0-alpha.42 made it break.

Here's the relevant Svelte source code for the error: https://github.com/sveltejs/svelte/blob/01a91163a9ffd6d18ea4699cef4c531b72fbfc00/src/runtime/internal/dev.ts#L161-L206 https://github.com/sveltejs/svelte/blob/815bc7ef6e91ea10e7de2c46325e32dd44939b84/test/runtime/index.ts#L304-L310

Svelte explicitly only throws this error in Dev mode, so it might not have anything to do with our dev vs build setup.

gbkwiatt commented 2 years ago

Exactly same here. So seems like more of a "global" issue rather than single example

kasperpeulen commented 2 years ago

Pff, super strange, I don't think I touched runtime code indeed.

I do import svelte here, which could maybe have an effect?

https://github.com/storybookjs/storybook/blob/75d5e18972ebe0325981210adbe520a5038fd41c/code/renderers/svelte/src/render.ts#L1-L11

This could be an import type { SvelteComponentTyped } from 'svelte', to make sure that didn't break anything.

shilman commented 2 years ago

Good golly!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-alpha.45 containing PR #19653 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.

shilman commented 2 years ago

@JReinhold @kasperpeulen any idea why this wasn't caught in CI?

IanVS commented 2 years ago

It only failed when starting storybook in dev mode, not when building. We don't have any tests that start storybook and navigate to stories, as far as I know.

shilman commented 2 years ago

Aha makes sense. @ndelangen @tmeasday any thoughts about this?

tmeasday commented 2 years ago

I guess it makes the case we should smoke test all sandboxes after all @ndelangen

ndelangen commented 2 years ago

That's not great news. So we have to figure out how to run all of them..

JReinhold commented 2 years ago

Just to be explicit, for us to have caught this bug in CI, we would need to:

  1. spin up a Storybook dev server
  2. Visit each story in a "browser"
  3. See that the preview is showing the error view

Non-trivial.

IanVS commented 2 years ago

I think the test runner could handle smoke testing the stories after launching the dev server, right? And honestly, since we're already testing each story in the prod build, it might be enough to just test one story in dev. Normally with this kind of issue they all break, not just one or two.

tmeasday commented 2 years ago

That's pretty fair, I think the test-runner at a dev server is a much better test than the smoke test. WDYT @ndelangen?