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]: `next/fonts/local` outputs with broken paths #21968

Closed amatlash closed 1 year ago

amatlash commented 1 year ago

Describe the bug

On Windows Storybook cant properly output next.js' next/fonts/local paths. My reproduction works perfectly fine with next dev and fails to get fonts with storybook dev/build.

Here's my next/font/local usage: image

And here's the output: image

Reproduction also provide example of more 'classic' approach for self-hosted fonts (with staticDirs prop) to be sure everything is alright with fonts.

To Reproduce

https://github.com/amatlash/storybook-next-font-path-bug-repro

System

Environment Info:

  System:
    OS: Windows 10 10.0.19045
    CPU: (16) x64 AMD Ryzen 7 5800U with Radeon Graphics
  Binaries:
    Node: 16.20.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.19.4 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (111.0.1661.62)    
  npmPackages:
    @storybook/addon-essentials: ^7.0.2 => 7.0.2 
    @storybook/addon-interactions: ^7.0.2 => 7.0.2 
    @storybook/addon-links: ^7.0.2 => 7.0.2 
    @storybook/blocks: ^7.0.2 => 7.0.2 
    @storybook/nextjs: ^7.0.2 => 7.0.2 
    @storybook/react: ^7.0.2 => 7.0.2 
    @storybook/testing-library: ^0.0.14-next.2 => 0.0.14-next.2

Additional context

Discord discussion

Chromatic build

pm0u commented 1 year ago

+1, we are experiencing this issue on a windows machine as well. It seems like the issue is related to this function: https://github.com/storybookjs/storybook/blob/1c3d36987dcc925d70a9959e496ad8ef5ec61258/code/frameworks/nextjs/src/font/webpack/loader/local/get-font-face-declarations.ts#L26-L47

which on a windows machine would create a "URL" like \\path\\to\\font\\file.woff2. Its not clear to me why both slashes disappear, however properly creating a URL path by relying on something other than path.join (which would create a filesystem path) should solve the issue.

possibly utilizing the path.sep constant would allow replacing this before passing it into the style tag. Or possibly directly importing from path.posix since the desired output is a posix-style path?

jeffballingerdev commented 1 year ago

I've also seen this issue on my windows 10 machine. The paths for the fonts in storybook resolve out to

src: url(....public ontsfile.woff2);

My storybook config file (main.ts) contains

staticDirs: [
    {
      from: "../public/fonts",
      to: "public/fonts",
    },
    "../public"
  ],
pm0u commented 1 year ago

regarding above, in the same project (we are working together on the same project) on a Mac the output I get is:

src: url(/public/fonts/file.woff2)

In our next font config, the path is defined as :

      path: "../../public/fonts/file.woff2",
Saeris commented 1 year ago

Also experiencing this issue on Windows. Was a workaround ever discovered?

This is a blocker to using next/.font/local with Storybook 7 on Windows right now.

For SEO purposes, this is the error message that is thrown:

NextFontError: Unexpected file `[object Object]`
Saeris commented 1 year ago

Okay, I did some further investigation based on @pm0u's comment regarding the getFontFaceDeclartions function.

I was curious to see what exactly Storybook was getting for my next/font loader options, so I added a simple console.log(options.props) before line 11 in this file:

https://github.com/storybookjs/storybook/blob/1c3d36987dcc925d70a9959e496ad8ef5ec61258/code/frameworks/nextjs/src/font/webpack/loader/local/get-font-face-declarations.ts#L9-L15

What that showed me was that my next/font/local loader was returning the following:

{
  src: [
    { path: {}, weight: {}, style: {} },
    { path: {}, weight: {}, style: {} },
    { path: {}, weight: {}, style: {} },
    { path: {}, weight: {}, style: {} }
  ],
  variable: {}
}

Okay that's obviously not right. Of course we'll get Unexpected file [object Object] in this case, since each font path is an empty object instead of a string! This causes the regex that gets executed from this import to come back falsey:

https://github.com/storybookjs/storybook/blob/1c3d36987dcc925d70a9959e496ad8ef5ec61258/code/frameworks/nextjs/src/font/webpack/loader/local/get-font-face-declarations.ts#L16

So that got me thinking... Next,js 13 has a bunch of hidden gotchas around the use of backticks for strings. I first discovered this regarding setting a runtime for edge functions. This turns out to be another such case (for me at least). Because all of this config for fonts must be known at build time, all of the values passed to these loader functions must be static, so you have to use single or double quotes for all strings here. In my case this requires turning off an ESLint rule that requires backtick usage.

Making that change resolved this particular error for me. I did run into one other error, and that was a ReferenceError: <identifier> is not defined. This was being caused by the use of import as syntax trying to import from next/font/google. Turns out you can do: import { Poppins as display } from "next/font/google" either, as this was the cause of this unrelated ReferenceError.

Here's ultimately what my font declaration file looks like after the fixes, with a note on what I had to change:

// NOTE: Every string passed to `next/font` functions *cannot*
// be dynamic! This eslint rule is disabled to prevent eslint --fix
// from automatically changing "" or '' strings to backick (``) strings
//
// Additionally, imports from `next/font/google` *cannot* be aliased with
// `as` (ex: `Poppins as display`). This causes Storybook to throw a
// separate error! Normally we would need to do something akin to this
// because of the eslint rules `new-cap` and `camelcase`, which also must
// be disabled here
/* eslint-disable new-cap, camelcase, @typescript-eslint/quotes */
import { Poppins, Nunito_Sans, Nunito } from "next/font/google";
import localFont from "next/font/local";

export const fontDisplay = Poppins({
  subsets: ["latin"],
  weight: ["200", "400", "600"],
  variable: "--font-display"
});

export const fontBody = Nunito_Sans({
  subsets: ["latin"],
  weight: ["400", "600"],
  variable: "--font-body"
});

export const fontCode = localFont({
  src: [
    {
      path: "../../public/fonts/Mona-Sans-Regular.woff2",
      weight: "400",
      style: "normal"
    },
    {
      path: "../../public/fonts/Mona-Sans-RegularItalic.woff2",
      weight: "400",
      style: "italic"
    },
    {
      path: "../../public/fonts/Mona-Sans-Bold.woff2",
      weight: "700",
      style: "normal"
    },
    {
      path: "../../public/fonts/Mona-Sans-BoldItalic.woff2",
      weight: "700",
      style: "italic"
    }
  ],
  variable: "--font-code"
});

export const fontInput = Nunito({
  subsets: ["latin"],
  weight: ["300", "400"],
  variable: "--font-input"
});

So, if you're running into this error, please double-check that you're not doing these subtle things wrong! Hope this helps.

rasoul707 commented 1 year ago

any solution?

valentinpalkovic commented 1 year ago

The solution got merged but isn't released yet.