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

[Bug]: Storybook 8.4 cannot serve static assets declared by file #29576

Closed
jackw opened this issue Nov 8, 2024 · 6 comments
Closed

[Bug]: Storybook 8.4 cannot serve static assets declared by file #29576

jackw opened this issue Nov 8, 2024 · 6 comments

Comments

@jackw
Copy link
Contributor

jackw commented Nov 8, 2024

Describe the bug

Storybook config accepts as staticDirs property objects of {from: 'path', to: 'path2'} but prior to storybook 8.4 it could also resolve directly to files. e.g.

    {
      from: "../some/directory/of/images/react-icon.svg",
      to: "/assets/images/react-icon.svg",
    },

Storybook 8.4 however cannot resolve the image and attempts to render a broken html page when opening the URL directly. We specify images directly in the staticDirs config property to limit the number of assets that are uploaded to a cloud bucket at release time as uploading the entire folder was causing rate limits to kick in with our cloud provider.

Reproduction link

https://github.com/jackw/storybook-84-asset-bug

Reproduction steps

  1. Clone the above repo (based on the chromaui/intro-storybook-react-template template)
  2. run yarn install
  3. run yarn storybook
  4. Note the theme brandImage should be the react icon.
  5. Switch to the branch storybook-8.4
  6. run yarn install
  7. run yarn storybook
  8. Note the theme branchImage is now a broken image.

System

Storybook Environment Info:

  System:
    OS: macOS 15.0.1
    CPU: (12) arm64 Apple M2 Pro
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.16.0 - ~/.nvm/versions/node/v20.16.0/bin/node
    Yarn: 1.22.22 - /opt/homebrew/bin/yarn <----- active
    npm: 10.8.1 - ~/.nvm/versions/node/v20.16.0/bin/npm
    pnpm: 9.12.2 - /opt/homebrew/bin/pnpm
  Browsers:
    Chrome: 130.0.6723.117
    Safari: 18.0.1
  npmPackages:
    @storybook/addon-essentials: ^8.4.2 => 8.4.2
    @storybook/addon-interactions: ^8.4.2 => 8.4.2
    @storybook/addon-links: ^8.4.2 => 8.4.2
    @storybook/blocks: ^8.4.2 => 8.4.2
    @storybook/react: ^8.4.2 => 8.4.2
    @storybook/react-vite: ^8.4.2 => 8.4.2
    @storybook/test: ^8.4.2 => 8.4.2
    eslint-plugin-storybook: ^0.11.0 => 0.11.0
    msw-storybook-addon: ^2.0.4 => 2.0.4
    storybook: ^8.4.2 => 8.4.2

Additional context

I haven't had the chance to dig too much into it but my instinct suggests this was caused by migrating from express to polka in #29230 and the use of sirv which appears to only handle directories.

@samvloeberghs
Copy link

samvloeberghs commented Nov 11, 2024

Additionally I would like to add that it appears we can't configure mappings originating from /node_modules anymore.
Just wanted to drop that here, will create a dedicated issue report in a few weeks if this is not really related.

@valentinpalkovic
Copy link
Contributor

@JReinhold Can this be related to the fs-extra removal?

@JReinhold
Copy link
Contributor

JReinhold commented Nov 11, 2024

I'm pretty sure OP is correct that this is related to sirv.

FWIW passing in file paths (as opposed to directory paths) has never been officially supported nor documented, I guess it just worked by chance because of the underlying implementation.

These are the docs I could find on staticDirs, and all of them exclusively mention directories:

@darondel-yoobic
Copy link
Contributor

darondel-yoobic commented Nov 13, 2024

We're also trying to upgrade from 8.3.6 to 8.4, and some assets are no longer served (build-storybook remains unchanged).
After digging into it, we found out assets are no longer merged. Let's say we have this kind of structure:

folder1/
  images/
    ...
folder2/
  images/
    ...
staticDirs: [
  { from: 'folder1', to: 'assets' }
  { from: 'folder2', to: 'assets' }
]

Only images from folder1 are served, for some reason. Images from folder2 get a 404 error. Removing the first static dir let us see images from folder2.

Do you think it's a regression? Is there any workaround/option to pass? It could be quite tedious for us to list every subfolder.

Not sure if the issue from OP is related, I can also open another issue.
Let me know if you need more details, happy to help!

@jackw
Copy link
Contributor Author

jackw commented Nov 15, 2024

FWIW passing in file paths (as opposed to directory paths) has never been officially supported nor documented, I guess it just worked by chance because of the underlying implementation.

Thanks @JReinhold , I did wonder if we were being a bit cheeky with the feature. 😆

For our needs I've opted to use fs-extra, copy what we need to a temp dir before starting storybook...

function copyAssetsSync() {
  const assets = [
    {
      from: '../../../public/fonts',
      to: './static/public/fonts',
    },
    {
      from: '../../../public/img/grafana_text_logo-dark.svg',
      to: './static/public/img/grafana_text_logo-dark.svg',
    },
    {
      from: '../../../public/img/grafana_text_logo-light.svg',
      to: './static/public/img/grafana_text_logo-light.svg',
    },
    {
      from: '../../../public/img/fav32.png',
      to: './static/public/img/fav32.png',
    },
  ];

  const staticDir = resolve(__dirname, 'static', 'public');

  emptyDirSync(staticDir);

  for (const asset of assets) {
    const fromPath = resolve(__dirname, asset.from);
    const toPath = resolve(__dirname, asset.to);

    copySync(fromPath, toPath, {
      filter: (src) => !lstatSync(src).isSymbolicLink(),
    });
  }
}

then set staticDirs: ['static'], in the storybook config to serve that path.

From my side I'd consider my original issue solved but not sure what maintainers want to do with this issue. Can either rename the title to encompass the other issues or close it?

@JReinhold
Copy link
Contributor

After digging into it, we found out assets are no longer merged. @darondel-yoobic

Similar to @jackw's problem statement, merging multiple directories into a single path was also never a feature that we supported, it just happened to work by chance. Even digging through the docs of express.static (the old underlying implementation of staticDirs) doesn't show any sign of it being an intentional feature. Therefore we don't consider it a regression, but simply an unsupported use case.

If listing out every static dir is tedious, the workaround is very similar to the one @jackw posted above, have a pre-start script that copies all your directories into a single one, and then serve that single directory instead. The downside of this workaround is that unless you set up a file watcher like chokidar, you'll have to restart your server whenever you make changes to any of those directories, but I'm not sure how often that is in a day-to-day workflow.

Closing this issue as won't fix. Thanks for reporting and thanks for your patience y'all. 🙏

@JReinhold JReinhold closed this as not planned Won't fix, can't repro, duplicate, stale Nov 20, 2024
@github-project-automation github-project-automation bot moved this from Empathy Backlog to Done in Core Team Projects Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

5 participants