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

cdnURL is not respected by google provider #224

Closed
haakonmt opened this issue Aug 15, 2024 · 10 comments · Fixed by #352
Closed

cdnURL is not respected by google provider #224

haakonmt opened this issue Aug 15, 2024 · 10 comments · Fixed by #352
Labels

Comments

@haakonmt
Copy link

haakonmt commented Aug 15, 2024

app.cdnURL (as the issue suggests) does not seem to be respected by the google provider, giving me 404s in production due to the font requests not being prepended correctly. Curiously, fonts provided by the local provider seem to be working as intended. I have not tried other providers, but as they look quite similar, I would presume this would also effect the other remote providers.

What makes it even more curious, is that if the fonts have already been cached (i.e. through a previous build), the paths are generated correctly (prefixed by cdnURL).

EDIT: Ok, so the reason it works after the fonts have been cached is that the remote fonts are now picked up by the local provider (since they have been cached locally), and thus served correctly.

@dosubot dosubot bot added the provider label Aug 15, 2024
Copy link

dosubot bot commented Aug 15, 2024

To resolve the issue where app.cdnURL is not respected by the Google provider, resulting in 404 errors in production due to font requests not being correctly prepended with the CDN URL, you need to modify the resolveFontFaceWithOverride function to prepend the CDN URL to the font requests.

Here's the modified resolveFontFaceWithOverride function:

async function resolveFontFaceWithOverride(fontFamily: string, override?: FontFamilyManualOverride | FontFamilyProviderOverride, fallbackOptions?: { fallbacks: string[], generic?: GenericCSSFamily }): Promise<FontFaceResolution | undefined> {
  const fallbacks = override?.fallbacks || normalizedDefaults.fallbacks[fallbackOptions?.generic || 'sans-serif']

  if (override && 'src' in override) {
    const fonts = normalizeFontData({
      src: override.src,
      display: override.display,
      weight: override.weight,
      style: override.style,
    })
    exposeFont({
      type: 'manual',
      fontFamily,
      fonts,
    })
    return {
      fallbacks,
      fonts,
    }
  }

  // Respect fonts that should not be resolved through `@nuxt/fonts`
  if (override?.provider === 'none') {
    return
  }

  // Respect custom weights, styles and subsets options
  const defaults = { ...normalizedDefaults, fallbacks }
  for (const key of ['weights', 'styles', 'subsets'] as const) {
    if (override?.[key]) {
      defaults[key as 'weights'] = override[key]!.map(v => String(v))
    }
  }

  // Handle explicit provider
  if (override?.provider) {
    if (override.provider in providers) {
      const result = await providers[override.provider]!.resolveFontFaces!(fontFamily, defaults)
      // Rewrite font source URLs to be proxied/local URLs
      const fonts = normalizeFontData(result?.fonts || [])
      if (!fonts.length || !result) {
        logger.warn(`Could not produce font face declaration from \`${override.provider}\` for font family \`${fontFamily}\`.`)
        return
      }
      exposeFont({
        type: 'override',
        fontFamily,
        provider: override.provider,
        fonts,
      })
      return {
        fallbacks: result.fallbacks || defaults.fallbacks,
        fonts,
      }
    }

    // If not registered, log and fall back to default providers
    logger.warn(`Unknown provider \`${override.provider}\` for font family \`${fontFamily}\`. Falling back to default providers.`)
  }

  for (const key of prioritisedProviders) {
    const provider = providers[key]!
    if (provider.resolveFontFaces) {
      const result = await provider.resolveFontFaces(fontFamily, defaults)
      if (result) {
        // Rewrite font source URLs to be proxied/local URLs
        const fonts = normalizeFontData(result.fonts)
        if (fonts.length > 0) {
          exposeFont({
            type: 'auto',
            fontFamily,
            provider: key,
            fonts,
          })
          // Prepend CDN URL to font requests
          const cdnURL = process.env.CDN_URL || 'https://your-cdn-url.com';
          fonts.forEach(font => {
            font.src = font.src.map(src => {
              if (src.url.startsWith('/')) {
                src.url = `${cdnURL}${src.url}`;
              }
              return src;
            });
          });
          return {
            fallbacks: result.fallbacks || defaults.fallbacks,
            fonts,
          }
        }
        if (override) {
          logger.warn(`Could not produce font face declaration for \`${fontFamily}\` with override.`)
        }
      }
    }
  }
}

In this modification, the fonts array is updated to prepend the CDN URL to all font requests. Make sure to set the CDN_URL environment variable to your CDN URL [1][2].

To continue talking to Dosu, mention @dosu.

Copy link

Would you be able to provide a reproduction? 🙏

More info

Why do I need to provide a reproduction?

Reproductions make it possible for us to triage and fix issues quickly with a relatively small team. It helps us discover the source of the problem, and also can reveal assumptions you or we might be making.

What will happen?

If you've provided a reproduction, we'll remove the label and try to reproduce the issue. If we can, we'll mark it as a bug and prioritise it based on its severity and how many people we think it might affect.

If needs reproduction labeled issues don't receive any substantial activity (e.g., new comments featuring a reproduction link), we'll close them. That's not because we don't care! At any point, feel free to comment with a reproduction and we'll reopen it.

How can I create a reproduction?

We have a template for starting with a minimal reproduction:

👉 https://stackblitz.com/github/nuxt/fonts/tree/main/example

A public GitHub repository is also perfect. 👌

Please ensure that the reproduction is as minimal as possible. See more details in our guide.

You might also find these other articles interesting and/or helpful:

@danielroe
Copy link
Member

danielroe commented Sep 23, 2024

possibly linked to #242 and possibly resolved with 9156b8a - would value a reproduction so I can investigate further 🙏

@haakonmt
Copy link
Author

@danielroe Still no dice on 0.9.1 it seems, I will try to provide a minimal reproduction when I have some spare time. Thanks for your effort. 🙌

@haakonmt
Copy link
Author

@danielroe Interesting observation on 0.9.1: On the first build, the raw font-faces produced in the server bundle looks like this:

@font-face {
  font-family: Inter;
  src: local("Inter Variable"), url(/_fonts/...);
  ...
}

On the second run (after the remote fonts have been cached), they look like this:

@font-face {
  font-family: Inter;
  src: local("Inter Variable"), url( publicAssetsURL("/_fonts/..."));
  ...
}

The publicAssetsURL helper seems to be doing what I need, it just isn't used on a clean build (which my CI builds are 😅).

@danielroe
Copy link
Member

Oh, that’s weird. Maybe a race condition. Probably a simple fix if so! 🤞

@haakonmt
Copy link
Author

@danielroe Sorry for the delay, but I have made a minimal reproduction of the issue here: https://github.com/haakonmt/fonts-cdn-reproduction

@danielroe
Copy link
Member

does it do this if you manually move them after the build, rather than using the nitro hook?

@haakonmt
Copy link
Author

@danielroe I could probably have found a better way to illustrate the issue, but yeah. IRL we don't use a hook, and deploy the assets after the build is complete, and it still breaks.

@haakonmt
Copy link
Author

haakonmt commented Oct 24, 2024

@danielroe Works like a charm now, thanks for the clever fix! 🙏

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

Successfully merging a pull request may close this issue.

2 participants