Font optimization bug fix (vercel#24162)

## Bug - [x] Related issues linked using `fixes vercel#23896` - [x] Integration tests added Fixes vercel#23896
nring · Apr 26, 2021 · fff183c · fff183c
1 parent fc53879
commit fff183c
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 43 deletions.
diff --git a/packages/next/next-server/lib/post-process.ts b/packages/next/next-server/lib/post-process.ts
@@ -13,24 +13,9 @@ type postProcessOptions = {
 type renderOptions = {
   getFontDefinition?: (url: string) => string
 }
-
-type PostProcessData = {
-  preloads: {
-    images: Array<string>
-  }
-}
-
 interface PostProcessMiddleware {
-  inspect: (
-    originalDom: HTMLElement,
-    data: PostProcessData,
-    options: renderOptions
-  ) => void
-  mutate: (
-    markup: string,
-    data: PostProcessData,
-    options: renderOptions
-  ) => Promise<string>
+  inspect: (originalDom: HTMLElement, options: renderOptions) => any
+  mutate: (markup: string, data: any, options: renderOptions) => Promise<string>
 }
 
 type middlewareSignature = {
@@ -58,18 +43,13 @@ async function processHTML(
   if (!middlewareRegistry[0]) {
     return html
   }
-  const postProcessData: PostProcessData = {
-    preloads: {
-      images: [],
-    },
-  }
   const root: HTMLElement = parse(html)
   let document = html
   // Calls the middleware, with some instrumentation and logging
   async function callMiddleWare(middleware: PostProcessMiddleware) {
     // let timer = Date.now()
-    middleware.inspect(root, postProcessData, data)
-    document = await middleware.mutate(document, postProcessData, data)
+    const inspectData = middleware.inspect(root, data)
+    document = await middleware.mutate(document, inspectData, data)
     // timer = Date.now() - timer
     // if (timer > MIDDLEWARE_TIME_BUDGET) {
     // TODO: Identify a correct upper limit for the postprocess step
@@ -89,15 +69,11 @@ async function processHTML(
 }
 
 class FontOptimizerMiddleware implements PostProcessMiddleware {
-  fontDefinitions: (string | undefined)[][] = []
-  inspect(
-    originalDom: HTMLElement,
-    _data: PostProcessData,
-    options: renderOptions
-  ) {
+  inspect(originalDom: HTMLElement, options: renderOptions) {
     if (!options.getFontDefinition) {
       return
     }
+    const fontDefinitions: (string | undefined)[][] = []
     // collecting all the requested font definitions
     originalDom
       .querySelectorAll('link')
@@ -115,30 +91,35 @@ class FontOptimizerMiddleware implements PostProcessMiddleware {
         const nonce = element.getAttribute('nonce')
 
         if (url) {
-          this.fontDefinitions.push([url, nonce])
+          fontDefinitions.push([url, nonce])
         }
       })
+
+    return fontDefinitions
   }
   mutate = async (
     markup: string,
-    _data: PostProcessData,
+    fontDefinitions: string[][],
     options: renderOptions
   ) => {
     let result = markup
     if (!options.getFontDefinition) {
       return markup
     }
-    for (const key in this.fontDefinitions) {
-      const [url, nonce] = this.fontDefinitions[key]
+
+    fontDefinitions.forEach((fontDef) => {
+      const [url, nonce] = fontDef
       const fallBackLinkTag = `<link rel="stylesheet" href="${url}"/>`
       if (
         result.indexOf(`<style data-href="${url}">`) > -1 ||
         result.indexOf(fallBackLinkTag) > -1
       ) {
         // The font is already optimized and probably the response is cached
-        continue
+        return
       }
-      const fontContent = options.getFontDefinition(url as string)
+      const fontContent = options.getFontDefinition
+        ? options.getFontDefinition(url as string)
+        : null
       if (!fontContent) {
         /**
          * In case of unreachable font definitions, fallback to default link tag.
@@ -151,13 +132,15 @@ class FontOptimizerMiddleware implements PostProcessMiddleware {
           `<style data-href="${url}"${nonceStr}>${fontContent}</style></head>`
         )
       }
-    }
+    })
+
     return result
   }
 }
 
 class ImageOptimizerMiddleware implements PostProcessMiddleware {
-  inspect(originalDom: HTMLElement, _data: PostProcessData) {
+  inspect(originalDom: HTMLElement) {
+    const imgPreloads = []
     const imgElements = originalDom.querySelectorAll('img')
     let eligibleImages: Array<HTMLElement> = []
     for (let i = 0; i < imgElements.length; i++) {
@@ -169,18 +152,18 @@ class ImageOptimizerMiddleware implements PostProcessMiddleware {
       }
     }
 
-    _data.preloads.images = []
-
     for (const imgEl of eligibleImages) {
       const src = imgEl.getAttribute('src')
       if (src) {
-        _data.preloads.images.push(src)
+        imgPreloads.push(src)
       }
     }
+
+    return imgPreloads
   }
-  mutate = async (markup: string, _data: PostProcessData) => {
+  mutate = async (markup: string, imgPreloads: string[]) => {
     let result = markup
-    let imagePreloadTags = _data.preloads.images
+    let imagePreloadTags = imgPreloads
       .filter((imgHref) => !preloadTagAlreadyExists(markup, imgHref))
       .reduce(
         (acc, imgHref) =>

diff --git a/test/integration/font-optimization/pages/with-font.js b/test/integration/font-optimization/pages/with-font.js
@@ -0,0 +1,30 @@
+import Head from 'next/head'
+import Link from 'next/link'
+
+const WithFont = () => {
+  return (
+    <>
+      <Head>
+        <link
+          href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;700;900&display=swap"
+          rel="stylesheet"
+        />
+      </Head>
+
+      <div>
+        Page with custom fonts
+        <br />
+        <br />
+        <Link href="/without-font">Without font</Link>
+      </div>
+    </>
+  )
+}
+
+export const getServerSideProps = async () => {
+  return {
+    props: {},
+  }
+}
+
+export default WithFont
diff --git a/test/integration/font-optimization/pages/without-font.js b/test/integration/font-optimization/pages/without-font.js
@@ -0,0 +1,26 @@
+import Head from 'next/head'
+import Link from 'next/link'
+
+const WithoutFont = () => {
+  return (
+    <>
+      <Head>
+        <meta name="robots" content="noindex, nofollow" />
+      </Head>
+      <div>
+        Page without custom fonts
+        <br />
+        <br />
+        <Link href="/with-font">With font</Link>
+      </div>
+    </>
+  )
+}
+
+export const getServerSideProps = async () => {
+  return {
+    props: {},
+  }
+}
+
+export default WithoutFont
diff --git a/test/integration/font-optimization/test/index.test.js b/test/integration/font-optimization/test/index.test.js
@@ -102,6 +102,25 @@ function runTests() {
     )
   })
 
+  it('should only inline included fonts per page', async () => {
+    const html = await renderViaHTTP(appPort, '/with-font')
+    expect(await fsExists(builtPage('font-manifest.json'))).toBe(true)
+    expect(html).toContain(
+      '<link rel="stylesheet" data-href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;700;900&amp;display=swap"/>'
+    )
+    expect(html).toMatch(
+      /<style data-href="https:\/\/fonts\.googleapis\.com\/css2\?family=Roboto:wght@400;700;900&display=swap">.*<\/style>/
+    )
+
+    const htmlWithoutFont = await renderViaHTTP(appPort, '/without-font')
+    expect(htmlWithoutFont).not.toContain(
+      '<link rel="stylesheet" data-href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;700;900&amp;display=swap"/>'
+    )
+    expect(htmlWithoutFont).not.toMatch(
+      /<style data-href="https:\/\/fonts\.googleapis\.com\/css2\?family=Roboto:wght@400;700;900&display=swap">.*<\/style>/
+    )
+  })
+
   it.skip('should minify the css', async () => {
     const snapshotJson = JSON.parse(
       await fs.readFile(join(__dirname, 'manifest-snapshot.json'), {