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]: After upgrade to v8.1.1 svg loader doesn't work for nextjs #27195

Closed
SalOne22 opened this issue May 18, 2024 · 8 comments · Fixed by #27198
Closed

[Bug]: After upgrade to v8.1.1 svg loader doesn't work for nextjs #27195

SalOne22 opened this issue May 18, 2024 · 8 comments · Fixed by #27198
Assignees

Comments

@SalOne22
Copy link

Describe the bug

After I upgraded storybook version from v8.0.10 to v8.1.1 my custom svgr loader broke. I have setup based on official storybooks nextjs documentation:

import type { StorybookConfig } from "@storybook/nextjs";

const config: StorybookConfig = {
  // ...rest of the config
  webpackFinal: async (config) => {
    config.module = config.module || {};
    config.module.rules = config.module.rules || [];

    // This modifies the existing image rule to exclude .svg files
    // since you want to handle those files with @svgr/webpack
    const imageRule = config.module.rules.find((rule) =>
      rule?.["test"]?.test(".svg"),
    );
    if (imageRule) {
      imageRule["exclude"] = /\.svg$/;
    }

    // Configure .svg files to be loaded with @svgr/webpack
    config.module.rules.push({
      test: /\.svg$/,
      use: ["@svgr/webpack"],
    });

    return config;
  },
};

But I keeping receiving this error: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.

Reproduction link

https://codesandbox.io/p/devbox/storybook-next-svgr-pzn4gm?layout=%257B%2522sidebarPanel%2522%253A%2522EXPLORER%2522%252C%2522rootPanelGroup%2522%253A%257B%2522direction%2522%253A%2522horizontal%2522%252C%2522contentType%2522%253A%2522UNKNOWN%2522%252C%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522id%2522%253A%2522ROOT_LAYOUT%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522UNKNOWN%2522%252C%2522direction%2522%253A%2522vertical%2522%252C%2522id%2522%253A%2522clwcn9n4l0007356gxvhpe2n3%2522%252C%2522sizes%2522%253A%255B51.77874186550976%252C48.22125813449024%255D%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522EDITOR%2522%252C%2522direction%2522%253A%2522horizontal%2522%252C%2522id%2522%253A%2522EDITOR%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522EDITOR%2522%252C%2522id%2522%253A%2522clwcn9n4k0002356gnsfntpmf%2522%257D%255D%257D%252C%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522SHELLS%2522%252C%2522direction%2522%253A%2522horizontal%2522%252C%2522id%2522%253A%2522SHELLS%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522SHELLS%2522%252C%2522id%2522%253A%2522clwcn9n4k0004356gb4zj1d30%2522%257D%255D%252C%2522sizes%2522%253A%255B100%255D%257D%255D%257D%252C%257B%2522type%2522%253A%2522PANEL_GROUP%2522%252C%2522contentType%2522%253A%2522DEVTOOLS%2522%252C%2522direction%2522%253A%2522vertical%2522%252C%2522id%2522%253A%2522DEVTOOLS%2522%252C%2522panels%2522%253A%255B%257B%2522type%2522%253A%2522PANEL%2522%252C%2522contentType%2522%253A%2522DEVTOOLS%2522%252C%2522id%2522%253A%2522clwcn9n4k0006356gfq8gaoju%2522%257D%255D%252C%2522sizes%2522%253A%255B100%255D%257D%255D%252C%2522sizes%2522%253A%255B50%252C50%255D%257D%252C%2522tabbedPanels%2522%253A%257B%2522clwcn9n4k0002356gnsfntpmf%2522%253A%257B%2522id%2522%253A%2522clwcn9n4k0002356gnsfntpmf%2522%252C%2522activeTabId%2522%253A%2522clwcng7oe00a8356grmipax1h%2522%252C%2522tabs%2522%253A%255B%257B%2522id%2522%253A%2522clwcn9n4k0001356gicqnqtxc%2522%252C%2522mode%2522%253A%2522permanent%2522%252C%2522type%2522%253A%2522FILE%2522%252C%2522filepath%2522%253A%2522%252FREADME.md%2522%257D%252C%257B%2522type%2522%253A%2522FILE%2522%252C%2522filepath%2522%253A%2522%252Fnextjs%252Fdefault-ts%252Fnext.config.mjs%2522%252C%2522id%2522%253A%2522clwcng7oe00a8356grmipax1h%2522%252C%2522mode%2522%253A%2522temporary%2522%257D%255D%257D%252C%2522clwcn9n4k0006356gfq8gaoju%2522%253A%257B%2522id%2522%253A%2522clwcn9n4k0006356gfq8gaoju%2522%252C%2522tabs%2522%253A%255B%255D%257D%252C%2522clwcn9n4k0004356gb4zj1d30%2522%253A%257B%2522id%2522%253A%2522clwcn9n4k0004356gb4zj1d30%2522%252C%2522tabs%2522%253A%255B%257B%2522id%2522%253A%2522clwcnagkn003i356gmbbd59fe%2522%252C%2522mode%2522%253A%2522permanent%2522%252C%2522type%2522%253A%2522TERMINAL%2522%252C%2522shellId%2522%253A%2522clwcnaifj003odbfm7tt5abe6%2522%257D%255D%252C%2522activeTabId%2522%253A%2522clwcnagkn003i356gmbbd59fe%2522%257D%257D%252C%2522showDevtools%2522%253Atrue%252C%2522showShells%2522%253Atrue%252C%2522showSidebar%2522%253Atrue%252C%2522sidebarPanelSize%2522%253A15%257D

Reproduction steps

  1. Go to above link
  2. Open preview (I hope it works)
  3. Go to Example/Icon story
  4. See build error

System

Storybook Environment Info:

  System:
    OS: Linux 6.1 Debian GNU/Linux 12 (bookworm) 12 (bookworm)
    CPU: (2) x64 AMD EPYC
    Shell: Unknown
  Binaries:
    Node: 20.9.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn <----- active
    npm: 9.8.1 - /usr/local/bin/npm
    pnpm: 8.10.2 - /usr/local/share/npm-global/bin/pnpm
  npmPackages:
    @storybook/addon-essentials: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/addon-interactions: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/addon-links: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/addon-onboarding: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/blocks: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/nextjs: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/react: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    @storybook/test: ^8.2.0-alpha.1 => 8.2.0-alpha.2 
    storybook: ^8.2.0-alpha.1 => 8.2.0-alpha.2

Additional context

This may be caused by:
Next.js: Avoid conflicts with the raw loader - #27093

@shilman
Copy link
Member

shilman commented May 19, 2024

@valentinpalkovic I don't think we should be patching PRs like #27093 back to main unless we are 99% confident they won't cause issues like this. Our prereleases are stable enough and release cycle is fast enough that people who really need the fix can use the prerelease and those who can't don't have to wait too long to get it in the next minor.

@valentinpalkovic
Copy link
Contributor

I will do some investigations on Tuesday, but on the first look I can’t see any relation to the mentioned PR.

@SalOne22 Is the issue reproducible on 8.1.0?

@SalOne22
Copy link
Author

No issue on 8.1.0, all works as expected

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented May 19, 2024

Ok. So, I figured out what the issue is.

Indeed, #27093 causes the issue for you because you don't mutate every loader, which matches .svg but only the first one found. We have edited one of the existing loaders:

{
  imageRule: {
    resourceQuery: /raw/,
    type: 'asset/source',
++    test: /^(?!__barrel_optimize__)/
  }
}

Therefore, this rule will be your first finding. Indeed, we have several loaders that would process svg files, and therefore you have to mutate all of them:

webpackFinal: async (config) => {
    config.module = config.module || {};
    config.module.rules = config.module.rules || [];

    // This modifies the existing image rule to exclude .svg files
    // since you want to handle those files with @svgr/webpack
-    const imageRule = config.module.rules.find((rule) =>
-      rule?.["test"]?.test(".svg"),
-    );
-    if (imageRule) {
-      imageRule["exclude"] = /\.svg$/;
-    }
+    config.module.rules.forEach((rule) => {
+      if (rule?.["test"]?.test(".svg")) {
+        rule!["exclude"] = /\.svg$/;
+      }
+
+      return rule;
+    });

    // Configure .svg files to be loaded with @svgr/webpack
    config.module.rules.push({
      test: /\.svg$/,
      use: ["@svgr/webpack"],
    });

    return config;
  },

I hope this helps.

@shilman It doesn't matter whether we would have released this in 8.2 or 8.1.1. This was a custom webpackFinal mutation, which we don't test during QA.

@shilman
Copy link
Member

shilman commented May 19, 2024

@valentinpalkovic It absolutely matters. If it's in the prerelease the community has up to two months to test it before it goes out. We've d found plenty of corner cases this way in the past.

@SalOne22
Copy link
Author

Okay, it works now. Thanks for the fast reply! The documentation should be updated to include this change so new users will not get confused by this: https://storybook.js.org/docs/get-started/nextjs#custom-webpack-config.

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented May 19, 2024

Excellent point!

@SalOne22 I just got one thing in mind: One downside of my approach is that as soon as svg files are imported in CSS files, the @svgr loader will transform them to React components as well. For Next.js, @svgr recommends mutating the webpack configuration to this: https://react-svgr.com/docs/next/. I even don't know how to do that for Turbopack. So essentially, if you want to use SVG in CSS files, you do not have to exclude the loader for all SVGs, but only for ones where the issuer is not CSS.

Something like this:

webpackFinal: async (config) => {
    config.module = config.module || {};
    config.module.rules = config.module.rules || [];

    // This modifies the existing image rule to exclude .svg files
    // since you want to handle those files with @svgr/webpack
-    const imageRule = config.module.rules.find((rule) =>
-      rule?.["test"]?.test(".svg"),
-    );
-    if (imageRule) {
-      imageRule["exclude"] = /\.svg$/;
-    }
+    config.module.rules.forEach((rule) => {
+      if (rule?.["test"]?.test(".svg") && rule?.issuer?.not?.test(".css")) {
+        rule!["exclude"] = /\.svg$/;
+      }
+
+      return rule;
+    });

    // Configure .svg files to be loaded with @svgr/webpack
    config.module.rules.push({
      test: /\.svg$/,
      use: ["@svgr/webpack"],
    });

    return config;
  },

@jonniebigodes, Can you take care of the documentation? Please ping me if you want to adjust the documentation. I would say we have to document it differently for pure Webpack5 and Next.js projects.

@jonniebigodes
Copy link
Contributor

@valentinpalkovic appreciate you tagging me on this. I'll take a look at this next week and follow up with you when the pull request is up. Sounds like a plan?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants