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

Fix dynamic routes for generateImageMetadata #48928

Merged
merged 9 commits into from Apr 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -106,9 +106,9 @@ export async function GET(_, ctx) {
const id = __metadata_id__[0]
const imageMetadata = generateImageMetadata ? await generateImageMetadata({ params }) : null
if (imageMetadata) {
const hasId = imageMetadata.some((item) => { item.id === id })
const hasId = imageMetadata.some((item) => item.id.toString() === id)
Copy link
Member

@styfle styfle Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case when item.id won't exist?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's required to have id, I'll think of some error message to add later when id is missing in any item

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just throw here id isn’t existing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, gonna check and throw in dev later

if (!hasId) {
return new NextResponse(null, {
return new NextResponse('Not Found', {
status: 404,
})
}
Expand Down Expand Up @@ -140,7 +140,7 @@ export async function GET(_, ctx) {
const targetId = __metadata_id__[0]
id = sitemaps.find((item) => item.id.toString() === targetId)?.id
if (id == null) {
return new NextResponse(null, {
return new NextResponse('Not Found', {
status: 404,
})
}
Expand Down
63 changes: 30 additions & 33 deletions packages/next/src/lib/metadata/get-metadata-route.ts
@@ -1,7 +1,6 @@
import { isMetadataRoute, isMetadataRouteFile } from './is-metadata-route'
import path from '../../shared/lib/isomorphic/path'
import { djb2Hash } from '../../shared/lib/hash'
import { isDynamicRoute } from '../../shared/lib/router/utils'

/*
* If there's special convention like (...) or @ in the page path,
Expand Down Expand Up @@ -30,42 +29,40 @@ export function getMetadataRouteSuffix(page: string) {
* @returns
*/
export function normalizeMetadataRoute(page: string) {
if (!isMetadataRoute(page)) {
return page
}
let route = page
let suffix = ''
if (isMetadataRoute(page)) {
if (route === '/robots') {
route += '.txt'
} else if (route === '/manifest') {
route += '.webmanifest'
} else if (route.endsWith('/sitemap')) {
route += '.xml'
} else {
// Remove the file extension, e.g. /route-path/robots.txt -> /route-path
const pathnamePrefix = page.slice(0, -(path.basename(page).length + 1))
suffix = getMetadataRouteSuffix(pathnamePrefix)
}
// Support both /<metadata-route.ext> and custom routes /<metadata-route>/route.ts.
// If it's a metadata file route, we need to append /[id]/route to the page.
if (!route.endsWith('/route')) {
const isStaticMetadataFile = isMetadataRouteFile(route, [], true)
const { dir, name: baseName, ext } = path.parse(route)
if (route === '/robots') {
route += '.txt'
} else if (route === '/manifest') {
route += '.webmanifest'
} else if (route.endsWith('/sitemap')) {
route += '.xml'
} else {
// Remove the file extension, e.g. /route-path/robots.txt -> /route-path
const pathnamePrefix = page.slice(0, -(path.basename(page).length + 1))
suffix = getMetadataRouteSuffix(pathnamePrefix)
}
// Support both /<metadata-route.ext> and custom routes /<metadata-route>/route.ts.
// If it's a metadata file route, we need to append /[id]/route to the page.
if (!route.endsWith('/route')) {
const isStaticMetadataFile = isMetadataRouteFile(page, [], true)
const { dir, name: baseName, ext } = path.parse(route)

// If it's dynamic routes, we need to append [[...__metadata_id__]] to the page;
// If it's static routes, we need to append nothing to the page.
// If its special routes like robots.txt and manifest.webmanifest, we leave them as static routes.
const isStaticRoute =
!isDynamicRoute(route) &&
(page.startsWith('/robots') ||
page.startsWith('/manifest') ||
isStaticMetadataFile)
const isStaticRoute =
page.startsWith('/robots') ||
page.startsWith('/manifest') ||
isStaticMetadataFile

route = path.posix.join(
dir,
`${baseName}${suffix ? `-${suffix}` : ''}${ext}`,
isStaticRoute ? '' : '[[...__metadata_id__]]',
'route'
)
}
route = path.posix.join(
dir,
`${baseName}${suffix ? `-${suffix}` : ''}${ext}`,
isStaticRoute ? '' : '[[...__metadata_id__]]',
'route'
)
}

return route
}
22 changes: 11 additions & 11 deletions packages/next/src/lib/metadata/is-metadata-route.ts
Expand Up @@ -43,7 +43,7 @@ export function isMetadataRouteFile(
new RegExp(
`^[\\\\/]robots${
withExtension
? `\\.${getExtensionRegexString(pageExtensions.concat('txt'))}`
? `\\.${getExtensionRegexString(pageExtensions.concat('txt'))}$`
: ''
}`
),
Expand All @@ -52,51 +52,51 @@ export function isMetadataRouteFile(
withExtension
? `\\.${getExtensionRegexString(
pageExtensions.concat('webmanifest', 'json')
)}`
)}$`
: ''
}`
),
new RegExp(`^[\\\\/]favicon\\.ico$`),
new RegExp(
`[\\\\/]sitemap${
withExtension
? `\\.${getExtensionRegexString(pageExtensions.concat('xml'))}`
? `\\.${getExtensionRegexString(pageExtensions.concat('xml'))}$`
: ''
}`
),
new RegExp(
`[\\\\/]${STATIC_METADATA_IMAGES.icon.filename}${
`[\\\\/]${STATIC_METADATA_IMAGES.icon.filename}\\d?${
huozhi marked this conversation as resolved.
Show resolved Hide resolved
withExtension
? `\\.${getExtensionRegexString(
pageExtensions.concat(STATIC_METADATA_IMAGES.icon.extensions)
)}`
)}$`
: ''
}`
),
new RegExp(
`[\\\\/]${STATIC_METADATA_IMAGES.apple.filename}${
`[\\\\/]${STATIC_METADATA_IMAGES.apple.filename}\\d?${
withExtension
? `\\.${getExtensionRegexString(
pageExtensions.concat(STATIC_METADATA_IMAGES.apple.extensions)
)}`
)}$`
: ''
}`
),
new RegExp(
`[\\\\/]${STATIC_METADATA_IMAGES.openGraph.filename}${
`[\\\\/]${STATIC_METADATA_IMAGES.openGraph.filename}\\d?${
withExtension
? `\\.${getExtensionRegexString(
pageExtensions.concat(STATIC_METADATA_IMAGES.openGraph.extensions)
)}`
)}$`
: ''
}`
),
new RegExp(
`[\\\\/]${STATIC_METADATA_IMAGES.twitter.filename}${
`[\\\\/]${STATIC_METADATA_IMAGES.twitter.filename}\\d?${
withExtension
? `\\.${getExtensionRegexString(
pageExtensions.concat(STATIC_METADATA_IMAGES.twitter.extensions)
)}`
)}$`
: ''
}`
),
Expand Down
@@ -0,0 +1,38 @@
import { ImageResponse } from 'next/server'

export const alt = 'Open Graph'

export async function generateImageMetadata() {
return [
{
contentType: 'image/png',
size: { width: 1200, height: 600 },
id: 100,
},
{
contentType: 'image/png',
size: { width: 1200, height: 600 },
id: 101,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets try a test when id is missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed as this will be easily forgotten.

]
}

export default function og() {
return new ImageResponse(
(
<div
style={{
width: '100%',
height: '100%',
display: 'flex',
alignItems: 'center',
justifyContent: 'center',
fontSize: 128,
background: 'lavender',
}}
>
Open Graph
</div>
)
)
}
25 changes: 17 additions & 8 deletions test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
Expand Up @@ -326,16 +326,25 @@ createNextDescribe(
it('should use localhost for local prod and fallback to deployment url when metadataBase is falsy', async () => {
const $ = await next.render$('/metadata-base/unset')
const twitterImage = $('meta[name="twitter:image"]').attr('content')

if (isNextDeploy) {
expect(twitterImage).toMatch(
/https:\/\/[\w-]+.vercel.app\/metadata-base\/unset\/twitter-image\.png/
const ogImages = $('meta[property="og:image"]')

expect(ogImages.length).toBe(2)
ogImages.each((_, ogImage) => {
huozhi marked this conversation as resolved.
Show resolved Hide resolved
const ogImageUrl = $(ogImage).attr('content')
expect(ogImageUrl).toMatch(
isNextDeploy
? /https:\/\/[\w-]+.vercel.app/
: /http:\/\/localhost:\d+/
)
} else {
expect(twitterImage).toMatch(
/http:\/\/localhost:\d+\/metadata-base\/unset\/twitter-image\.png/
expect(ogImageUrl).toMatch(
/\/metadata-base\/unset\/opengraph-image2\/10\d/
)
}
})

expect(twitterImage).toMatch(
isNextDeploy ? /https:\/\/[\w-]+.vercel.app/ : /http:\/\/localhost:\d+/
)
expect(twitterImage).toMatch(/\/metadata-base\/unset\/twitter-image\.png/)
})

if (isNextStart) {
Expand Down