Skip to content

Commit

Permalink
Fix favicon merging with customized icons (#67982)
Browse files Browse the repository at this point in the history
### What

Support merging the static metadata file conventions `favicon.ico` with
other customized metadata icon from the module export.


### Why

`favicon.ico` should be displayed everywhere as it's the icon
representative of the website for all pages. Any customized `icon` in
metadata export `export const metadata` or `export function
generateMetadata()` shouldn't override the `favicon.ico` file
convention.


Fixes #55767

---------

Co-authored-by: hrmny <[email protected]>
  • Loading branch information
huozhi and ForsakenHarmony authored Aug 7, 2024
1 parent 070159a commit 9568a4f
Show file tree
Hide file tree
Showing 11 changed files with 171 additions and 50 deletions.
13 changes: 1 addition & 12 deletions crates/next-core/src/app_structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -871,17 +871,6 @@ async fn directory_tree_to_loader_tree(
.then_some(components.page)
.flatten()
{
// When resolving metadata with corresponding module
// (https:/vercel/next.js/blob/aa1ee5995cdd92cc9a2236ce4b6aa2b67c9d32b2/packages/next/src/lib/metadata/resolve-metadata.ts#L340)
// layout takes precedence over page (https:/vercel/next.js/blob/aa1ee5995cdd92cc9a2236ce4b6aa2b67c9d32b2/packages/next/src/server/lib/app-dir-module.ts#L22)
// If the component have layout and page both, do not attach same metadata to
// the page.
let metadata = if components.layout.is_some() {
Default::default()
} else {
components.metadata.clone()
};

tree.parallel_routes.insert(
"children".into(),
LoaderTree {
Expand All @@ -890,7 +879,7 @@ async fn directory_tree_to_loader_tree(
parallel_routes: IndexMap::new(),
components: Components {
page: Some(page),
metadata,
metadata: components.metadata,
..Default::default()
}
.cell(),
Expand Down
6 changes: 5 additions & 1 deletion crates/next-core/src/loader_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,11 @@ impl LoaderTreeBuilder {
metadata: &Metadata,
global_metadata: Option<&GlobalMetadata>,
) -> Result<()> {
if metadata.is_empty() {
if metadata.is_empty()
&& global_metadata
.map(|global| global.is_empty())
.unwrap_or_default()
{
return Ok(());
}
let Metadata {
Expand Down
103 changes: 71 additions & 32 deletions packages/next/src/lib/metadata/resolve-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@ import type { OpenGraph } from './types/opengraph-types'
import type { ComponentsType } from '../../build/webpack/loaders/next-app-loader'
import type { MetadataContext } from './types/resolvers'
import type { LoaderTree } from '../../server/lib/app-dir-module'
import type { AbsoluteTemplateString } from './types/metadata-types'
import type {
AbsoluteTemplateString,
IconDescriptor,
} from './types/metadata-types'
import type { ParsedUrlQuery } from 'querystring'
import type { StaticMetadata } from './types/icons'

import {
createDefaultMetadata,
Expand Down Expand Up @@ -45,8 +49,6 @@ import { ResolveMetadataSpan } from '../../server/lib/trace/constants'
import { PAGE_SEGMENT_KEY } from '../../shared/lib/segment'
import * as Log from '../../build/output/log'

type StaticMetadata = Awaited<ReturnType<typeof resolveStaticMetadata>>

type MetadataResolver = (
parent: ResolvingMetadata
) => Metadata | Promise<Metadata>
Expand Down Expand Up @@ -78,44 +80,48 @@ type PageProps = {
searchParams: { [key: string]: any }
}

function hasIconsProperty(
icons: Metadata['icons'],
prop: 'icon' | 'apple'
): boolean {
if (!icons) return false
if (prop === 'icon') {
// Detect if icons.icon will be presented, icons array and icons string will all be merged into icons.icon
return !!(
typeof icons === 'string' ||
icons instanceof URL ||
Array.isArray(icons) ||
(prop in icons && icons[prop])
)
} else {
// Detect if icons.apple will be presented, only icons.apple will be merged into icons.apple
return !!(typeof icons === 'object' && prop in icons && icons[prop])
function isFavicon(icon: IconDescriptor | undefined): boolean {
if (!icon) {
return false
}

// turbopack appends a hash to all images
return (
(icon.url === '/favicon.ico' ||
icon.url.toString().startsWith('/favicon.ico?')) &&
icon.type === 'image/x-icon'
)
}

function mergeStaticMetadata(
source: Metadata | null,
target: ResolvedMetadata,
staticFilesMetadata: StaticMetadata,
metadataContext: MetadataContext,
titleTemplates: TitleTemplates
titleTemplates: TitleTemplates,
isLastSegment: boolean
) {
if (!staticFilesMetadata) return
const { icon, apple, openGraph, twitter, manifest } = staticFilesMetadata
// file based metadata is specified and current level metadata icons is not specified
if (
(icon && !hasIconsProperty(source?.icons, 'icon')) ||
(apple && !hasIconsProperty(source?.icons, 'apple'))
) {
target.icons = {
icon: icon || [],
apple: apple || [],

// Only pick up the static metadata if the current level is the last segment
if (isLastSegment) {
// file based metadata is specified and current level metadata icons is not specified
if (target.icons) {
if (icon) {
target.icons.icon.unshift(...icon)
}
if (apple) {
target.icons.apple.unshift(...apple)
}
} else if (icon || apple) {
target.icons = {
icon: icon || [],
apple: apple || [],
}
}
}

// file based metadata is specified and current level metadata twitter.images is not specified
if (twitter && !source?.twitter?.hasOwnProperty('images')) {
const resolvedTwitter = resolveTwitter(
Expand Down Expand Up @@ -152,13 +158,15 @@ function mergeMetadata({
titleTemplates,
metadataContext,
buildState,
isLastSegment,
}: {
source: Metadata | null
target: ResolvedMetadata
staticFilesMetadata: StaticMetadata
titleTemplates: TitleTemplates
metadataContext: MetadataContext
buildState: BuildState
isLastSegment: boolean
}): void {
// If there's override metadata, prefer it otherwise fallback to the default metadata.
const metadataBase =
Expand Down Expand Up @@ -281,7 +289,8 @@ function mergeMetadata({
target,
staticFilesMetadata,
metadataContext,
titleTemplates
titleTemplates,
isLastSegment
)
}

Expand Down Expand Up @@ -383,7 +392,10 @@ async function collectStaticImagesFiles(
: undefined
}

async function resolveStaticMetadata(components: ComponentsType, props: any) {
async function resolveStaticMetadata(
components: ComponentsType,
props: any
): Promise<StaticMetadata> {
const { metadata } = components
if (!metadata) return null

Expand Down Expand Up @@ -573,6 +585,7 @@ function inheritFromMetadata(
const commonOgKeys = ['title', 'description', 'images'] as const
function postProcessMetadata(
metadata: ResolvedMetadata,
favicon: any,
titleTemplates: TitleTemplates,
metadataContext: MetadataContext
): ResolvedMetadata {
Expand Down Expand Up @@ -629,6 +642,17 @@ function postProcessMetadata(
inheritFromMetadata(openGraph, metadata)
inheritFromMetadata(twitter, metadata)

if (favicon) {
if (!metadata.icons) {
metadata.icons = {
icon: [],
apple: [],
}
}

metadata.icons.icon.unshift(favicon)
}

return metadata
}

Expand Down Expand Up @@ -681,7 +705,7 @@ async function getMetadataFromExport<Data, ResolvedData>(
// Only preload at the beginning when resolves are empty
if (!dynamicMetadataResolvers.length) {
for (let j = currentIndex; j < metadataItems.length; j++) {
const preloadMetadataExport = getPreloadMetadataExport(metadataItems[j]) // metadataItems[j][0]
const preloadMetadataExport = getPreloadMetadataExport(metadataItems[j])
// call each `generateMetadata function concurrently and stash their resolver
if (typeof preloadMetadataExport === 'function') {
collectMetadataExportPreloading<Data, ResolvedData>(
Expand Down Expand Up @@ -748,9 +772,18 @@ export async function accumulateMetadata(
const buildState = {
warnings: new Set<string>(),
}

let favicon
for (let i = 0; i < metadataItems.length; i++) {
const staticFilesMetadata = metadataItems[i][1]

// Treat favicon as special case, it should be the first icon in the list
// i <= 1 represents root layout, and if current page is also at root
if (i <= 1 && isFavicon(staticFilesMetadata?.icon?.[0])) {
const iconMod = staticFilesMetadata?.icon?.shift()
if (i === 0) favicon = iconMod
}

const metadata = await getMetadataFromExport<Metadata, ResolvedMetadata>(
(metadataItem) => metadataItem[0],
dynamicMetadataResolvers,
Expand All @@ -767,6 +800,7 @@ export async function accumulateMetadata(
staticFilesMetadata,
titleTemplates,
buildState,
isLastSegment: i === metadataItems.length - 1,
})

// If the layout is the same layer with page, skip the leaf layout and leaf page
Expand All @@ -787,7 +821,12 @@ export async function accumulateMetadata(
}
}

return postProcessMetadata(resolvedMetadata, titleTemplates, metadataContext)
return postProcessMetadata(
resolvedMetadata,
favicon,
titleTemplates,
metadataContext
)
}

export async function accumulateViewport(
Expand Down
7 changes: 7 additions & 0 deletions packages/next/src/lib/metadata/types/icons.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export type StaticMetadata = {
icon: any[] | undefined
apple: any[] | undefined
openGraph: any[] | undefined
twitter: any[] | undefined
manifest: string | undefined
} | null
Binary file added test/e2e/app-dir/metadata-icons/app/favicon.ico
Binary file not shown.
20 changes: 20 additions & 0 deletions test/e2e/app-dir/metadata-icons/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { ReactNode } from 'react'

export default function Root({ children }: { children: ReactNode }) {
return (
<html>
<body>{children}</body>
</html>
)
}

export const metadata = {
icons: {
shortcut: '/shortcut-icon.png',
apple: '/apple-icon.png',
other: {
rel: 'apple-touch-icon-precomposed',
url: '/apple-touch-icon-precomposed.png',
},
},
}
14 changes: 14 additions & 0 deletions test/e2e/app-dir/metadata-icons/app/nested/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
export default function Page() {
return <p>hello world</p>
}

export const metadata = {
icons: {
shortcut: '/shortcut-icon-nested.png',
apple: '/apple-icon-nested.png',
other: {
rel: 'apple-touch-icon-precomposed-nested',
url: '/apple-touch-icon-precomposed-nested.png',
},
},
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/metadata-icons/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <p>hello world</p>
}
31 changes: 31 additions & 0 deletions test/e2e/app-dir/metadata-icons/metadata-icons.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { nextTestSetup } from 'e2e-utils'

describe('app-dir - metadata-icons', () => {
const { next } = nextTestSetup({
files: __dirname,
})

it('should only have 1 favicon link in root page', async () => {
const $ = await next.render$('/')
expect($('link[href^="/favicon.ico"]').length).toBe(1)
})

it('should only have 1 favicon link in nested page', async () => {
const $ = await next.render$('/nested')
expect($('link[href^="/favicon.ico"]').length).toBe(1)
})

it('should render custom icons along with favicon in root page', async () => {
const $ = await next.render$('/')
expect($('link[rel="shortcut icon"]').attr('href')).toBe(
'/shortcut-icon.png'
)
})

it('should render custom icons along with favicon in nested page', async () => {
const $ = await next.render$('/nested')
expect($('link[rel="shortcut icon"]').attr('href')).toBe(
'/shortcut-icon-nested.png'
)
})
})
16 changes: 11 additions & 5 deletions test/e2e/app-dir/metadata/metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,10 +428,13 @@ describe('app dir - metadata', () => {
'twitter:title': 'no-tw-image',
})

// icon should be overridden
expect($('link[rel="icon"]').attr('href')).toBe(
'https://custom-icon-1.png'
)
// icon should be overridden and contain favicon.ico
const [favicon, ...icons] = $('link[rel="icon"]')
.toArray()
.map((i) => $(i).attr('href'))

expect(favicon).toMatch('/favicon.ico')
expect(icons).toEqual(['https://custom-icon-1.png'])
})
})

Expand All @@ -456,6 +459,7 @@ describe('app dir - metadata', () => {

await checkLink(browser, 'shortcut icon', '/shortcut-icon.png')
await checkLink(browser, 'icon', [
expect.stringMatching(/favicon\.ico/),
'/icon.png',
'https://example.com/icon.png',
])
Expand Down Expand Up @@ -553,7 +557,9 @@ describe('app dir - metadata', () => {
expect($appleIcon.length).toBe(0)

const $dynamic = await next.render$('/icons/static/dynamic-routes/123')
const $dynamicIcon = $dynamic('head > link[rel="icon"]')
const $dynamicIcon = $dynamic(
'head > link[rel="icon"][type!="image/x-icon"]'
)
const dynamicIconHref = $dynamicIcon.attr('href')
expect(dynamicIconHref).toMatch(
/\/icons\/static\/dynamic-routes\/123\/icon/
Expand Down
8 changes: 8 additions & 0 deletions test/integration/app-dir-export/test/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export const expectedWhenTrailingSlashTrue = [
'404.html',
'404/index.html',
// Turbopack and plain next.js have different hash output for the file name
// Turbopack will output favicon in the _next/static/media folder
...(process.env.TURBOPACK
? [expect.stringMatching(/_next\/static\/media\/favicon\.[0-9a-f]+\.ico/)]
: []),
expect.stringMatching(/_next\/static\/media\/test\.[0-9a-f]+\.png/),
'_next/static/test-build-id/_buildManifest.js',
...(process.env.TURBOPACK
Expand All @@ -58,6 +62,10 @@ export const expectedWhenTrailingSlashTrue = [

const expectedWhenTrailingSlashFalse = [
'404.html',
// Turbopack will output favicon in the _next/static/media folder
...(process.env.TURBOPACK
? [expect.stringMatching(/_next\/static\/media\/favicon\.[0-9a-f]+\.ico/)]
: []),
expect.stringMatching(/_next\/static\/media\/test\.[0-9a-f]+\.png/),
'_next/static/test-build-id/_buildManifest.js',
...(process.env.TURBOPACK
Expand Down

0 comments on commit 9568a4f

Please sign in to comment.