storybookjs / storybook

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

Relative URLs to assets no longer work in CSS now that CSS no longer gets inlined #4645

Closed Vinnl closed 4 years ago

Vinnl commented 5 years ago

Describe the bug I just upgraded to Storybook 4, and I think this issue is caused by that (it required a lot of other moving parts as well).

Basically, in my React component, I import './Widget.css';. Then in Widget.css, I reference a background image located in the same folder as such: background-image: url(orcid_16x16.png);.

With Storybook 3, this just worked even using build-storybook: the image URL was updated to static/media/orcid_16x16.2dddb203.png, and since the CSS was inline, this path was correct.

Now with Storybook 4, however, the URL is still updated to that value, but because it is now located in a CSS file that itself located in static/css, the browser is now trying to find the image at static/css/static/media/, and hence cannot find it.

To Reproduce Steps to reproduce the behavior:

  1. Visit my built Storybook v3
  2. Notice that there is a green icon to the left-hand side of Alice Doe
  3. Now visit that same page in Storybook 4
  4. Notice that the image is missing

Expected behavior The image to also be present in Storybook 4 - presumably by making sure that URL imports are rewritten to be relative to the CSS file they are in.

Screenshots

In Storybook 3: image

In Storybook 4: image

System:

igor-dv commented 5 years ago

Do you have any custom webpack.config?

Vinnl commented 5 years ago

@igor-dv Ah, yes, sorry - I'm using TypeScript.

The config in the case of the working images:

const path = require("path");

module.exports = (baseConfig, env, defaultConfig) => {
  defaultConfig.module.rules.push({
    test: /\.(ts|tsx)$/,
    include: [
      path.resolve(__dirname, "../src"),
      path.resolve(__dirname, "../stories"),
    ],
    loader: require.resolve("ts-loader"),
    options: { configFile: '../tsconfig.json' }
  });
  defaultConfig.resolve.extensions.push(".ts", ".tsx");

  return defaultConfig;
};

and the config in the case where images do not load:

const path = require("path");

module.exports = (baseConfig, env, defaultConfig) => {
  defaultConfig.module.rules.push({
    test: /\.(ts|tsx)$/,
    include: [
      path.resolve(__dirname, "../src"),
      path.resolve(__dirname, "../stories"),
    ],
    loader: require.resolve('awesome-typescript-loader'),
    options: { configFileName: path.resolve(__dirname, './tsconfig.json') }
  });
  defaultConfig.resolve.extensions.push(".ts", ".tsx");

  return defaultConfig;
};

The difference between the two is that the loader changed from ts-loader to awesome-typescript-loader.

igor-dv commented 5 years ago

So the difference in the awesome-typescript-loader vs ts-loader, right ? 🤔

Vinnl commented 5 years ago

Yes, as far as the Webpack config is concerned. Though i don't think they do anything with image imports, especially when done from CSS?

igor-dv commented 5 years ago

Did you try to use SB v4 with the ts-loader (to eliminate this problem)? Nothing is changed in png loading in a new release...

Vinnl commented 5 years ago

Well, I wanted to, but unfortunately I couldn't get it to work - I ran into this error message:

Module build failed (from ./node_modules/ts-loader/index.js):
Error: TypeScript emitted no output for <..>/stories/widget.tsx.
at makeSourceMapAndFinish (<..>/node_modules/ts-loader/dist/index.js:78:15) 
at successLoader (<..>/node_modules/ts-loader/dist/index.js:68:9)
at Object.loader (<..>/node_modules/ts-loader/dist/index.js:22:12)

Since the official docs also use awesome-typescript-loader, I figured I'd try it with that, and that worked. (I used to use ts-loader because my app used that as well, but that is no longer the case.)

igor-dv commented 5 years ago

BTW, how does your tsconfig look like? (I am asking different questions because idk what is the problem 🤷‍♂️ )

Also, if you could share a public reproduction, that'd be great.

Vinnl commented 5 years ago

Haha, I was afraid so :P

The tsconfig.json is not that special:

{
  "compilerOptions": {
    "target": "es5",
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react",
    "lib": ["esnext", "dom"]
  },
  "include": [
    "src"
  ],
  "exclude": [
    "src/setupProxy.js"
  ]
}

I don't have a minimal reproduction unfortunately, but the project this occurs on luckily is open source: with Storybook 3, with Storybook 4. (Note that there are several projects in that repo - the one with Storybook is located in /client.)

igor-dv commented 5 years ago

You can exclude stories from tsconfig and create another one for storybook (tsconfig.sb.json) that will extend your regular tsconfig. In this new tsconfig you can remove the "noEmit": true, (it could be a root of your initial ts-loader problem). Excluding stories from the tsconfig could boost your apps building time.

Vinnl commented 5 years ago

@igor-dv Hehe, that's what I was doing already - I just mashed them together here for easier display :)

Setting noEmit: false did solve ts-loader not working. Unfortunately, images are still not loaded :(

igor-dv commented 5 years ago

Looks like I need to clone the project 😴

Vinnl commented 5 years ago

Ah don't bother, that's far too much effort! I'll probably be diving into this again tomorrow; who knows, I might get some fresh insights :)

igor-dv commented 5 years ago

I see that you've migrated to react-scripts as well. We have a feature that integrates styling from the CRA2.

CC: @chadfawcett , could it be somehow related to this?

chadfawcett commented 5 years ago

I believe this is because you're switching to CRA. I believe the file paths need to be relative. So change orcid_16x16.png to ./orcid_16x16.png.

https://facebook.github.io/create-react-app/docs/adding-images-fonts-and-files https://github.com/facebook/create-react-app/issues/4574#issuecomment-395081558

chadfawcett commented 5 years ago

Although, I wasn't actually able to replicate the issue. I cloned the repo, checked out the branch, and ran yarn storybook. This may be because I'm on MacOS, and it does a bit of magic when it comes to resolving file paths.

screen shot 2018-10-31 at 10 50 29 am

Vinnl commented 5 years ago

@chadfawcett Actually, it used to be like that - I just changed it to see whether that fixes it. I'll try changing it back, but I think orcid_16x16.png is relative as well - especially considering that this URL is changed in the built version, just not to the correct path.

Note that it does work locally (i.e. unbuilt) for me as well, because the CSS is still inline at that point, so the relative import path still works. It just doesn't work in the built version because the CSS file is then in a subfolder, and imports are resolved by the browser relative to that.

Edit: Just tried it with a ./ import - the images still won't load, unnfortunately.

Thanks for your help both, though, you're really going above and beyond :)

chadfawcett commented 5 years ago

Sorry @Vinnl, I missed the fact that this was specific to built version.

Looks like it's a bug in the storybook cra webpack preset. I'll try to get a PR in tonight.

Vinnl commented 5 years ago

That'd be fantastic @chadfawcett. If you have any pointers I'd be happy to take a shot at it myself, if need be :)

Vinnl commented 5 years ago

Some more observations: CRA successfully turns it into an absolute URL, as can be seen here. However, it knows how to do so because the developer configures at what location the app will be deployed. Given that that location is different for the built Storybook application, I'm not sure how to deal with that other than providing a similar configuration option for Storybook itself.

Though perhaps an alternative is to always inline images as data URLs, given that data use is probably (?) less of an issue for Storybooks. Or, alternatively, not to extract the CSS into separate files at all, like in Storybook 3.

Anyway, I'm not sure if that is desirable from a Storybook point of view, so that's probably best for you to judge.

chadfawcett commented 5 years ago

@Vinnl Apologies that this took longer than expected. I got caught up with some testing issues.

Take a look at #4695. Would you be able to clone my branch and test it with your repo? I've already done so, but it would be good if you could confirm the fix resolves your issue. Here's how to include the local build in your project https://github.com/storybooks/storybook/blob/master/CONTRIBUTING.md#working-with-your-own-app

Vinnl commented 5 years ago

No problem @chadfawcett, I'm already indebted to you for even trying to fix it, let alone actually doing so :)

Anyway, I saw the PR and was already trying it out - I've just confirmed that it does, indeed, fix the issue :)

Vinnl commented 5 years ago

Just upgraded to 4.0.4, and it works flawlessly. Thanks!

tulsidaskhatri commented 4 years ago

I am getting exactly same issue with "@storybook/react": "^5.3.7",

css is trying to look for a font file in static/css/static/media/myfont.woff2

but the font is at following url: static/media/myfont.woff2

ppiyush13 commented 4 years ago

I am also facing same issue

VinceVachon commented 4 years ago

Same "5.3.9",

shilman commented 4 years ago

@mrmckeb did something change in the CRA preset that might affect this?

mrmckeb commented 4 years ago

This looks like a conflict between Storybook and CRA's configurations, which is odd. I can take a look at this.

Has anyone got a reproduction they can share with me? Thanks!

YDundie commented 4 years ago

Same for the version 5.3.13

YDundie commented 4 years ago

Hello. Successfully solved it by adding this to my .storybook/main.js

const path = require('path');

const publicPath = process.env.DEV ? "http://localhost:9009/" :"https://your-web-address.com/"

module.exports = {
    stories: ['../src/**/*.stories.js'],
    addons: [
        '@storybook/preset-create-react-app',
        '@storybook/addon-actions',
        '@storybook/addon-links',
        '@storybook/addon-knobs'],

    webpackFinal: async (config, { configType }) => {

        config.output.publicPath=publicPath

        return config;
    },
};

Hopefully it could help you.

cherouvim commented 4 years ago

Has anyone got a reproduction they can share with me? Thanks!

Hello. This is the most minimal repro I could come up with. Steps:

  1. Confirm that the woff font file is loading via the network tab:
    npm install
    npm run storybook
  2. Confirm that font file is not loading (404s due to static/css/static/media):
    npm run build-storybook
    # view generated index.html

storybook-repro-4645.zip

maniax89 commented 4 years ago

in my project - i was able to remedy this by adding the "homepage": "." to my package json -> https://create-react-app.dev/docs/deployment/#serving-the-same-build-from-different-paths

not sure if this helps anyone^

seems like the relevant lines in CRA look something like this (https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/config/webpack.config.js):

  // common function to get style loaders
  const getStyleLoaders = (cssOptions, preProcessor) => {
    const loaders = [
      isEnvDevelopment && require.resolve('style-loader'),
      isEnvProduction && {
        loader: MiniCssExtractPlugin.loader,
        // css is located in `static/css`, use '../../' to locate index.html folder
        // in production `paths.publicUrlOrPath` can be a relative path
        options: paths.publicUrlOrPath.startsWith('.')
          ? { publicPath: '../../' }
          : {},
      },

and public path is set below:

      // webpack uses `publicPath` to determine where the app is being served from.
      // It requires a trailing slash, or the file assets will get an incorrect path.
      // We inferred the "public path" (such as / or /my-project) from homepage.
      publicPath: paths.publicUrlOrPath,
shilman commented 4 years ago

@mrmckeb any chance you can look at @cherouvim 's repro?

mrmckeb commented 4 years ago

@cherouvim, I downloaded your project and ran it - and I see "Rumba" loading as expected.

image

You also have a webpack.config.js file which is not needed, as CRA inserts the necessary config.

I then migrated the project to use the new preset for CRA, and everything was still working correctly.

Can you please provide some more info? Thanks!

mrmckeb commented 4 years ago

@YDundie, I'd like to find the root cause of this and fix it in the preset. Are you able to share an example of what isn't working for you?

Even just code snippets here.

cherouvim commented 4 years ago

@mrmckeb it does work on npm run storybook, but does it work on npm run build-storybook?

mrmckeb commented 4 years ago

Thanks @cherouvim, I didn't understand that it was build only. Found and hopefully fixed. Thanks for the solution @maniax89!

ZeeshanTamboli commented 4 years ago

@maniax89's solution to add "homepage": "." in package.json worked for me. Thanks!

rodoabad commented 3 years ago

This is still happening on non-CRA apps.

In CSS.

// foo.module.scss
background: url('./logo.svg') 30px center no-repeat $white;

After Storybook.

static/media/logo.f6b1d730.svg
kneupper commented 2 years ago

For anybody still having this issue down the road on a non-CRA app, please reference this SO thread

The key of this is in specifying { url: false } in the CSS Module rule of your webpack config. Here is the specific configuration I used for my CSS Module rule:

config.module.rules.push({
    test: /\.module\.css$/,
    use: [
        require.resolve('style-loader'),
        {
            loader: require.resolve('css-loader'),
            options: {
                modules: {...}, // project-specific module naming conventions, config, etc.
                url: false,
                importLoaders: 1,
            },
        },
    ],
}) 

Hope this can save some time for anybody else!