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

Always collect static icons for all segments #68712

Merged
merged 7 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 40 additions & 21 deletions packages/next/src/lib/metadata/resolve-metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type { LoaderTree } from '../../server/lib/app-dir-module'
import type {
AbsoluteTemplateString,
IconDescriptor,
ResolvedIcons,
} from './types/metadata-types'
import type { ParsedUrlQuery } from 'querystring'
import type { StaticMetadata } from './types/icons'
Expand Down Expand Up @@ -49,6 +50,8 @@ import { ResolveMetadataSpan } from '../../server/lib/trace/constants'
import { PAGE_SEGMENT_KEY } from '../../shared/lib/segment'
import * as Log from '../../build/output/log'

type StaticIcons = Pick<ResolvedIcons, 'icon' | 'apple'>

type MetadataResolver = (
parent: ResolvingMetadata
) => Metadata | Promise<Metadata>
Expand Down Expand Up @@ -99,27 +102,18 @@ function mergeStaticMetadata(
staticFilesMetadata: StaticMetadata,
metadataContext: MetadataContext,
titleTemplates: TitleTemplates,
isLastSegment: boolean
leafSegmentStaticIcons: StaticIcons
) {
if (!staticFilesMetadata) return
const { icon, apple, openGraph, twitter, manifest } = staticFilesMetadata

// 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 || [],
}
}
// Keep updating the static icons in the most leaf node

if (icon) {
leafSegmentStaticIcons.icon = icon
}
if (apple) {
leafSegmentStaticIcons.apple = apple
}

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

Expand Down Expand Up @@ -774,6 +768,13 @@ export async function accumulateMetadata(
}

let favicon

// Collect the static icons in the most leaf node,
// since we don't collect all the static metadata icons in the parent segments.
const leafSegmentStaticIcons = {
icon: [],
apple: [],
}
for (let i = 0; i < metadataItems.length; i++) {
const staticFilesMetadata = metadataItems[i][1]

Expand All @@ -800,7 +801,7 @@ export async function accumulateMetadata(
staticFilesMetadata,
titleTemplates,
buildState,
isLastSegment: i === metadataItems.length - 1,
leafSegmentStaticIcons,
})

// If the layout is the same layer with page, skip the leaf layout and leaf page
Expand All @@ -814,6 +815,24 @@ export async function accumulateMetadata(
}
}

if (
leafSegmentStaticIcons.icon.length > 0 ||
leafSegmentStaticIcons.apple.length > 0
) {
if (!resolvedMetadata.icons) {
resolvedMetadata.icons = {
icon: [],
apple: [],
}
if (leafSegmentStaticIcons.icon.length > 0) {
resolvedMetadata.icons.icon.unshift(...leafSegmentStaticIcons.icon)
}
if (leafSegmentStaticIcons.apple.length > 0) {
resolvedMetadata.icons.apple.unshift(...leafSegmentStaticIcons.apple)
}
}
}

// Only log warnings if there are any, and only once after the metadata resolving process is finished
if (buildState.warnings.size > 0) {
for (const warning of buildState.warnings) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Modal() {
return <p>modal</p>
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
14 changes: 14 additions & 0 deletions test/e2e/app-dir/metadata-icons-parallel-routes/app/icon.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
10 changes: 10 additions & 0 deletions test/e2e/app-dir/metadata-icons-parallel-routes/app/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
export default function Root({ children, modal }) {
return (
<html>
<body>
{modal}
{children}
</body>
</html>
)
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <p>page</p>
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/metadata-icons-parallel-routes/app/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <p>page</p>
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { nextTestSetup } from 'e2e-utils'

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

it('should present favicon with other icons when parallel routes are presented', async () => {
const $ = await next.render$('/')
expect($('link[type="image/x-icon"]').length).toBe(1)
expect($('link[type="image/svg+xml"]').length).toBe(1)
expect($('link[rel="apple-touch-icon"]').length).toBe(1)
})

it('should override parent icon when both static icon presented', async () => {
const $ = await next.render$('/nested')
expect($('link[rel="icon"][type="image/png"]').length).toBe(1)
huozhi marked this conversation as resolved.
Show resolved Hide resolved
})

it('should inherit parent apple icon when child does not present but parent contain static apple icon', async () => {
const $ = await next.render$('/nested')
expect($('link[rel="apple-touch-icon"][type="image/png"]').length).toBe(1)
})
})
Loading