Error for missing dynamic generated id and refactor metadata image url (#48953)

### What? In #48928 we decided to error for the missing `id` from `generateImageMetadata` and `generateSitemaps` for better dev DX. This PR also refactors the metadata image urls generation that assumbling the utils together
2023-04-28 16:06:07 +02:00 · 2023-04-28 16:06:07 +02:00 · a141366ff6
commit a141366ff6
parent cc2bcece63
5 changed files with 223 additions and 135 deletions
--- a/packages/next/src/build/webpack/loaders/metadata/discover.ts
+++ b/packages/next/src/build/webpack/loaders/metadata/discover.ts
@ -6,7 +6,6 @@ import type {
 import path from 'path'
 import { stringify } from 'querystring'
 import { STATIC_METADATA_IMAGES } from '../../../../lib/metadata/is-metadata-route'
-import { normalizeAppPath } from '../../../../shared/lib/router/utils/app-paths'

 const METADATA_TYPE = 'metadata'

@ -125,7 +124,6 @@ export async function createStaticMetadataFromRoute(
          {
            type,
            segment,
-            route: normalizeAppPath(segment),
            pageExtensions,
          }
        )}!${filepath}${METADATA_RESOURCE_QUERY}`
--- a/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts
+++ b/packages/next/src/build/webpack/loaders/next-metadata-image-loader.ts
@ -14,7 +14,6 @@ import { imageExtMimeTypeMap } from '../../../lib/mime-type'
 import { fileExists } from '../../../lib/file-exists'

 interface Options {
-  route: string
  segment: string
  type: PossibleImageFileNameConvention
  pageExtensions: string[]
@ -22,7 +21,7 @@ interface Options {

 async function nextMetadataImageLoader(this: any, content: Buffer) {
  const options: Options = this.getOptions()
-  const { type, route, segment, pageExtensions } = options
+  const { type, segment, pageExtensions } = options
  const numericSizes = type === 'twitter' || type === 'openGraph'
  const { resourcePath, rootContext: context } = this
  const { name: fileNameBase, ext } = path.parse(resourcePath)
@ -47,36 +46,31 @@ async function nextMetadataImageLoader(this: any, content: Buffer) {
  )

  const isDynamicResource = pageExtensions.includes(extension)
-  const pageRoute = isDynamicResource ? fileNameBase : interpolatedName
+  const pageSegment = isDynamicResource ? fileNameBase : interpolatedName
  const hashQuery = contentHash ? '?' + contentHash : ''

  if (isDynamicResource) {
    // re-export and spread as `exportedImageData` to avoid non-exported error
    return `\
-    import path from 'next/dist/shared/lib/isomorphic/path'
    import * as exported from ${JSON.stringify(resourcePath)}
-    import { interpolateDynamicPath } from 'next/dist/server/server-utils'
-    import { getNamedRouteRegex } from 'next/dist/shared/lib/router/utils/route-regex'
-    import { getMetadataRouteSuffix } from 'next/dist/lib/metadata/get-metadata-route'
+    import { fillMetadataSegment } from 'next/dist/lib/metadata/get-metadata-route'

    const imageModule = { ...exported }
    export default async function (props) {
-      const pathname = ${JSON.stringify(route)}
-      const routeRegex = getNamedRouteRegex(pathname, false)
-      const segment = ${JSON.stringify(segment)}
      const { __metadata_id__: _, ...params } = props.params
-      const route = interpolateDynamicPath(pathname, params, routeRegex)
-      const suffix = getMetadataRouteSuffix(segment)
-      const routeSuffix = suffix ? \`-\${suffix}\` : ''
-      const imageRoute = ${JSON.stringify(pageRoute)} + routeSuffix
+      const imageUrl = fillMetadataSegment(${JSON.stringify(
+        segment
+      )}, params, ${JSON.stringify(pageSegment)})

      const { generateImageMetadata } = imageModule

-      function getImageMetadata(imageMetadata, segment) {
+      function getImageMetadata(imageMetadata, idParam) {
        const data = {
          alt: imageMetadata.alt,
          type: imageMetadata.contentType || 'image/png',
-          url: path.join(route, segment + ${JSON.stringify(hashQuery)}),
+          url: imageUrl + (idParam ? ('/' + idParam) : '') + ${JSON.stringify(
+            hashQuery
+          )},
        }
        const { size } = imageMetadata
        if (size) {
@ -92,11 +86,11 @@ async function nextMetadataImageLoader(this: any, content: Buffer) {
      if (generateImageMetadata) {
        const imageMetadataArray = await generateImageMetadata({ params })
        return imageMetadataArray.map((imageMetadata, index) => {
-          const segment = path.join(imageRoute, (imageMetadata.id || index) + '')
-          return getImageMetadata(imageMetadata, segment)
+          const idParam = (imageMetadata.id || index) + ''
+          return getImageMetadata(imageMetadata, idParam)
        })
      } else {
-        return [getImageMetadata(imageModule, imageRoute)]
+        return [getImageMetadata(imageModule, '')]
      }
    }`
  }
@ -137,27 +131,17 @@ async function nextMetadataImageLoader(this: any, content: Buffer) {
  }

  return `\
-  import path from 'next/dist/shared/lib/isomorphic/path'
-  import { interpolateDynamicPath } from 'next/dist/server/server-utils'
-  import { getNamedRouteRegex } from 'next/dist/shared/lib/router/utils/route-regex'
-  import { getMetadataRouteSuffix } from 'next/dist/lib/metadata/get-metadata-route'
+  import { fillMetadataSegment } from 'next/dist/lib/metadata/get-metadata-route'

  export default (props) => {
-    const pathname = ${JSON.stringify(route)}
-    const routeRegex = getNamedRouteRegex(pathname, false)
-    const segment = ${JSON.stringify(segment)}
-    const route = interpolateDynamicPath(pathname, props.params, routeRegex)
-    const suffix = getMetadataRouteSuffix(segment)
-    const routeSuffix = suffix ? \`-\${suffix}\` : ''
-    const { name, ext } = path.parse(${JSON.stringify(pageRoute)})
-
-    const imageData = ${JSON.stringify(imageData)};
+    const imageData = ${JSON.stringify(imageData)}
+    const imageUrl = fillMetadataSegment(${JSON.stringify(
+      segment
+    )}, props.params, ${JSON.stringify(pageSegment)})

    return [{
      ...imageData,
-      url: path.join(route, name + routeSuffix + ext + ${JSON.stringify(
-        type === 'favicon' ? '' : hashQuery
-      )}),
+      url: imageUrl + ${JSON.stringify(type === 'favicon' ? '' : hashQuery)},
    }]
  }`
 }
--- a/packages/next/src/build/webpack/loaders/next-metadata-route-loader.ts
+++ b/packages/next/src/build/webpack/loaders/next-metadata-route-loader.ts
@ -103,11 +103,20 @@ const generateImageMetadata = imageModule.generateImageMetadata

 export async function GET(_, ctx) {
  const { __metadata_id__ = [], ...params } = ctx.params
-  const id = __metadata_id__[0]
+  const targetId = __metadata_id__[0]
+  let id = undefined
  const imageMetadata = generateImageMetadata ? await generateImageMetadata({ params }) : null
+
  if (imageMetadata) {
-    const hasId = imageMetadata.some((item) => item.id.toString() === id)
-    if (!hasId) {
+    id = imageMetadata.find((item) => {
+      if (process.env.NODE_ENV !== 'production') {
+        if (item?.id == null) {
+          throw new Error('id is required for every item returned from generateImageMetadata')
+        }
+      }
+      return item.id.toString() === targetId
+    })?.id
+    if (id == null) {
      return new NextResponse('Not Found', {
        status: 404,
      })
@ -132,13 +141,20 @@ const contentType = ${JSON.stringify(getContentType(resourcePath))}
 const fileType = ${JSON.stringify(getFilenameAndExtension(resourcePath).name)}

 export async function GET(_, ctx) {
-  const sitemaps = generateSitemaps ? await generateSitemaps() : null
+  const { __metadata_id__ = [], ...params } = ctx.params
+  const targetId = __metadata_id__[0]
  let id = undefined
+  const sitemaps = generateSitemaps ? await generateSitemaps() : null

  if (sitemaps) {
-    const { __metadata_id__ = [] } = ctx.params
-    const targetId = __metadata_id__[0]
-    id = sitemaps.find((item) => item.id.toString() === targetId)?.id
+    id = sitemaps.find((item) => {
+      if (process.env.NODE_ENV !== 'production') {
+        if (item?.id == null) {
+          throw new Error('id property is required for every item returned from generateSitemaps')
+        }
+      }
+      return item.id.toString() === targetId
+    })?.id
    if (id == null) {
      return new NextResponse('Not Found', {
        status: 404,
--- a/packages/next/src/lib/metadata/get-metadata-route.ts
+++ b/packages/next/src/lib/metadata/get-metadata-route.ts
@ -1,6 +1,9 @@
 import { isMetadataRoute, isMetadataRouteFile } from './is-metadata-route'
 import path from '../../shared/lib/isomorphic/path'
+import { interpolateDynamicPath } from '../../server/server-utils'
+import { getNamedRouteRegex } from '../../shared/lib/router/utils/route-regex'
 import { djb2Hash } from '../../shared/lib/hash'
+import { normalizeAppPath } from '../../shared/lib/router/utils/app-paths'

 /*
 * If there's special convention like (...) or @ in the page path,
@ -10,7 +13,7 @@ import { djb2Hash } from '../../shared/lib/hash'
 * /app/open-graph.tsx -> /open-graph/route
 * /app/(post)/open-graph.tsx -> /open-graph/route-[0-9a-z]{6}
 */
-export function getMetadataRouteSuffix(page: string) {
+function getMetadataRouteSuffix(page: string) {
  let suffix = ''

  if ((page.includes('(') && page.includes(')')) || page.includes('@')) {
@ -19,6 +22,29 @@ export function getMetadataRouteSuffix(page: string) {
  return suffix
 }

+/**
+ * Fill the dynamic segment in the metadata route
+ *
+ * Example:
+ * fillMetadataSegment('/a/[slug]', { params: { slug: 'b' } }, 'open-graph') -> '/a/b/open-graph'
+ *
+ */
+export function fillMetadataSegment(
+  segment: string,
+  params: any,
+  imageSegment: string
+) {
+  const pathname = normalizeAppPath(segment)
+  const routeRegex = getNamedRouteRegex(pathname, false)
+  const route = interpolateDynamicPath(pathname, params, routeRegex)
+  const suffix = getMetadataRouteSuffix(segment)
+  const routeSuffix = suffix ? `-${suffix}` : ''
+
+  const { name, ext } = path.parse(imageSegment)
+
+  return path.join(route, `${name}${routeSuffix}${ext}`)
+}
+
 /**
 * Map metadata page key to the corresponding route
 *
--- a/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
+++ b/test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
@ -1,5 +1,6 @@
 import { createNextDescribe } from 'e2e-utils'
 import imageSize from 'image-size'
+import { check } from 'next-test-utils'

 const CACHE_HEADERS = {
  NONE: 'no-cache, no-store',
@ -108,102 +109,97 @@ createNextDescribe(
        )
      })

-      if (!isNextDeploy) {
-        // TODO-APP: investigate the dynamic routes failing in deployment tests
-        it('should support generate multi images with generateImageMetadata', async () => {
-          const $ = await next.render$('/dynamic/big')
-          const iconUrls = $('link[rel="icon"]')
-            .toArray()
-            .map((el) => {
-              return {
-                href: $(el).attr('href').split('?')[0],
-                sizes: $(el).attr('sizes'),
-                type: $(el).attr('type'),
-              }
-            })
-          // slug is id param from generateImageMetadata
-          expect(iconUrls).toMatchObject([
-            {
-              href: '/dynamic/big/icon-48jo90/small',
-              sizes: '48x48',
-              type: 'image/png',
-            },
-            {
-              href: '/dynamic/big/icon-48jo90/medium',
-              sizes: '72x72',
-              type: 'image/png',
-            },
-          ])
+      it('should support generate multi images with generateImageMetadata', async () => {
+        const $ = await next.render$('/dynamic/big')
+        const iconUrls = $('link[rel="icon"]')
+          .toArray()
+          .map((el) => {
+            return {
+              href: $(el).attr('href').split('?')[0],
+              sizes: $(el).attr('sizes'),
+              type: $(el).attr('type'),
+            }
+          })
+        // slug is id param from generateImageMetadata
+        expect(iconUrls).toMatchObject([
+          {
+            href: '/dynamic/big/icon-48jo90/small',
+            sizes: '48x48',
+            type: 'image/png',
+          },
+          {
+            href: '/dynamic/big/icon-48jo90/medium',
+            sizes: '72x72',
+            type: 'image/png',
+          },
+        ])

-          const appleTouchIconUrls = $('link[rel="apple-touch-icon"]')
-            .toArray()
-            .map((el) => {
-              return {
-                href: $(el).attr('href').split('?')[0],
-                sizes: $(el).attr('sizes'),
-                type: $(el).attr('type'),
-              }
-            })
-          // slug is index by default
-          expect(appleTouchIconUrls).toEqual([
-            {
-              href: '/dynamic/big/apple-icon-48jo90/0',
-              sizes: '48x48',
-              type: 'image/png',
-            },
-            {
-              href: '/dynamic/big/apple-icon-48jo90/1',
-              sizes: '64x64',
-              type: 'image/png',
-            },
-          ])
-        })
+        const appleTouchIconUrls = $('link[rel="apple-touch-icon"]')
+          .toArray()
+          .map((el) => {
+            return {
+              href: $(el).attr('href').split('?')[0],
+              sizes: $(el).attr('sizes'),
+              type: $(el).attr('type'),
+            }
+          })
+        // slug is index by default
+        expect(appleTouchIconUrls).toEqual([
+          {
+            href: '/dynamic/big/apple-icon-48jo90/0',
+            sizes: '48x48',
+            type: 'image/png',
+          },
+          {
+            href: '/dynamic/big/apple-icon-48jo90/1',
+            sizes: '64x64',
+            type: 'image/png',
+          },
+        ])
+      })

-        it('should support generate multi sitemaps with generateSitemaps', async () => {
-          const ids = [0, 1, 2, 3]
-          function fetchSitemap(id) {
-            return next
-              .fetch(`/dynamic/small/sitemap.xml/${id}`)
-              .then((res) => res.text())
-          }
+      it('should support generate multi sitemaps with generateSitemaps', async () => {
+        const ids = [0, 1, 2, 3]
+        function fetchSitemap(id) {
+          return next
+            .fetch(`/dynamic/small/sitemap.xml/${id}`)
+            .then((res) => res.text())
+        }

-          for (const id of ids) {
-            const text = await fetchSitemap(id)
-            expect(text).toContain(
-              `<loc>https://example.com/dynamic/${id}</loc>`
-            )
-          }
-        })
+        for (const id of ids) {
+          const text = await fetchSitemap(id)
+          expect(text).toContain(`<loc>https://example.com/dynamic/${id}</loc>`)
+        }
+      })

-        it('should fill params into dynamic routes url of metadata images', async () => {
-          const $ = await next.render$('/dynamic/big')
-          const ogImageUrl = $('meta[property="og:image"]').attr('content')
-          expect(ogImageUrl).toMatch(hashRegex)
-          expect(ogImageUrl).toMatch('/dynamic/big/opengraph-image')
-          // should already normalize the parallel routes segment to url
-          expect(ogImageUrl).not.toContain('(group)')
-        })
+      it('should fill params into dynamic routes url of metadata images', async () => {
+        const $ = await next.render$('/dynamic/big')
+        const ogImageUrl = $('meta[property="og:image"]').attr('content')
+        expect(ogImageUrl).toMatch(hashRegex)
+        expect(ogImageUrl).toMatch('/dynamic/big/opengraph-image')
+        // should already normalize the parallel routes segment to url
+        expect(ogImageUrl).not.toContain('(group)')
+      })

-        it('should support params as argument in dynamic routes', async () => {
-          const big$ = await next.render$('/dynamic/big')
-          const small$ = await next.render$('/dynamic/small')
-          const bigOgUrl = new URL(
-            big$('meta[property="og:image"]').attr('content')
-          )
-          const smallOgUrl = new URL(
-            small$('meta[property="og:image"]').attr('content')
-          )
-          const bufferBig = await (await next.fetch(bigOgUrl.pathname)).buffer()
-          const bufferSmall = await (
-            await next.fetch(smallOgUrl.pathname)
-          ).buffer()
+      it('should support params as argument in dynamic routes', async () => {
+        const big$ = await next.render$('/dynamic/big')
+        const small$ = await next.render$('/dynamic/small')
+        const bigOgUrl = new URL(
+          big$('meta[property="og:image"]').attr('content')
+        )
+        const smallOgUrl = new URL(
+          small$('meta[property="og:image"]').attr('content')
+        )
+        const bufferBig = await (await next.fetch(bigOgUrl.pathname)).buffer()
+        const bufferSmall = await (
+          await next.fetch(smallOgUrl.pathname)
+        ).buffer()

-          const sizeBig = imageSize(bufferBig)
-          const sizeSmall = imageSize(bufferSmall)
-          expect([sizeBig.width, sizeBig.height]).toEqual([1200, 630])
-          expect([sizeSmall.width, sizeSmall.height]).toEqual([600, 315])
-        })
-      }
+        const sizeBig = imageSize(bufferBig)
+        const sizeSmall = imageSize(bufferSmall)
+        expect([sizeBig.width, sizeBig.height]).toEqual([1200, 630])
+        expect([sizeSmall.width, sizeSmall.height]).toEqual([600, 315])
+      })

      it('should fill params into routes groups url of static images', async () => {
        const $ = await next.render$('/static')
@ -347,6 +343,74 @@ createNextDescribe(
      expect(twitterImage).toMatch(/\/metadata-base\/unset\/twitter-image\.png/)
    })

+    if (isNextDev) {
+      it('should error when id is missing in generateImageMetadata', async () => {
+        const iconFilePath = 'app/metadata-base/unset/icon.tsx'
+        const contentMissingIdProperty = `
+        import { ImageResponse } from 'next/server'
+        export async function generateImageMetadata() {
+          return [
+            {
+              contentType: 'image/png',
+              size: { width: 48, height: 48 },
+              // id: 100,
+            },
+            {
+              contentType: 'image/png',
+              size: { width: 48, height: 48 },
+              id: 101,
+            },
+          ]
+        }
+
+        export default function icon() {
+          return new ImageResponse(<div>icon</div>)
+        }
+        `
+        await next.patchFile(iconFilePath, contentMissingIdProperty)
+        await next.fetch('/metadata-base/unset/icon/100')
+        await next.deleteFile(iconFilePath) // revert
+
+        await check(async () => {
+          expect(next.cliOutput).toContain(
+            `id is required for every item returned from generateImageMetadata`
+          )
+          return 'success'
+        }, /success/)
+      })
+
+      it('should error when id is missing in generateSitemaps', async () => {
+        const sitemapFilePath = 'app/metadata-base/unset/sitemap.tsx'
+        const contentMissingIdProperty = `
+        import { MetadataRoute } from 'next'
+
+        export async function generateSitemaps() {
+          return [
+            { id: 0 },
+          ]
+        }
+
+        export default function sitemap({ id }): MetadataRoute.Sitemap {
+          return [
+            {
+              url: 'https://example.com/',
+              lastModified: '2021-01-01',
+            },
+          ]
+        }`
+        await next.patchFile(sitemapFilePath, contentMissingIdProperty)
+        await next.fetch('/metadata-base/unset/sitemap.xml/0')
+        await next.deleteFile(sitemapFilePath) // revert
+
+        await check(async () => {
+          expect(next.cliOutput).toContain(
+            `id is required for every item returned from generateImageMetadata`
+          )
+          return 'success'
+        }, /success/)
+      })
+    }
+
    if (isNextStart) {
      it('should support edge runtime of image routes', async () => {
        const middlewareManifest = JSON.parse(