-
Notifications
You must be signed in to change notification settings - Fork 326
fix(metadata): apply ancestor templates to title defaults #1256
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -343,6 +343,29 @@ function resolveStringTitle(title: Metadata["title"]): string | undefined { | |
| return undefined; | ||
| } | ||
|
|
||
| function applyTitleTemplate(template: string | undefined, title: string): string { | ||
| return template ? template.replace(/%s/g, title) : title; | ||
| } | ||
|
|
||
| function resolveTitle(title: Metadata["title"], stashedTemplate: string | undefined) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The return type here is implicitly |
||
| if (typeof title === "string") { | ||
| return applyTitleTemplate(stashedTemplate, title); | ||
| } | ||
|
|
||
| if (title && typeof title === "object") { | ||
| let resolved = | ||
| title.default === undefined ? undefined : applyTitleTemplate(stashedTemplate, title.default); | ||
|
|
||
| if (title.absolute) { | ||
| resolved = title.absolute; | ||
| } | ||
|
|
||
| return resolved; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Post-process merged metadata to cross-fill openGraph and Twitter fields. | ||
| * | ||
|
|
@@ -400,7 +423,7 @@ export function postProcessMetadata(merged: Metadata): Metadata { | |
| if (!hasTwDescription) { | ||
| autoFill.description = result.openGraph.description || result.description || undefined; | ||
| } | ||
| if (!hasTwImages) { | ||
| if (!hasTwImages && result.openGraph.images !== undefined) { | ||
| autoFill.images = result.openGraph.images; | ||
| } | ||
|
|
||
|
|
@@ -456,25 +479,14 @@ export function mergeMetadataEntries(entries: readonly MetadataMergeEntry[]): Me | |
|
|
||
| const merged: Metadata = {}; | ||
|
|
||
| // Track the most recent title template from LAYOUTS (not from page). | ||
| // Track the most recent ancestor title template from layouts (not from page). | ||
| let parentTemplate: string | undefined; | ||
|
|
||
| for (const entry of entries) { | ||
| const meta = entry.metadata; | ||
| const isPage = Boolean(entry.isPage); | ||
| const contributesTitle = entry.contributesTitle !== false; | ||
|
|
||
| // Collect template from layouts only (page templates are ignored per Next.js spec) | ||
| if ( | ||
| contributesTitle && | ||
| !isPage && | ||
| meta.title && | ||
| typeof meta.title === "object" && | ||
| meta.title.template | ||
| ) { | ||
| parentTemplate = meta.title.template; | ||
| } | ||
|
|
||
| // Merge non-title keys | ||
| for (const key of Object.keys(meta)) { | ||
| if (key === "title") continue; // Handle title separately below | ||
|
|
@@ -492,30 +504,19 @@ export function mergeMetadataEntries(entries: readonly MetadataMergeEntry[]): Me | |
|
|
||
| // Title resolution | ||
| if (contributesTitle && meta.title !== undefined) { | ||
| merged.title = meta.title; | ||
| merged.title = resolveTitle(meta.title, parentTemplate); | ||
| } | ||
| } | ||
|
|
||
| // Now resolve the final title, applying the parent template if applicable | ||
| const finalTitle = merged.title; | ||
| if (finalTitle) { | ||
| if (typeof finalTitle === "string") { | ||
| // Simple string title — apply parent template | ||
| if (parentTemplate) { | ||
| merged.title = parentTemplate.replace("%s", finalTitle); | ||
| } | ||
| } else if (typeof finalTitle === "object") { | ||
| if (finalTitle.absolute) { | ||
| // Absolute title — skip all templates | ||
| merged.title = finalTitle.absolute; | ||
| } else if (finalTitle.default) { | ||
| // Title object with default — this is used when the segment IS the | ||
| // defining layout (its own default doesn't get template-wrapped) | ||
| merged.title = finalTitle.default; | ||
| } else if (finalTitle.template && !finalTitle.default && !finalTitle.absolute) { | ||
| // Template only with no default — no title to render | ||
| merged.title = undefined; | ||
| } | ||
| // Collect the current layout template after resolving its own title so | ||
| // title.default is wrapped by the ancestor template, not by its own template. | ||
| if ( | ||
| contributesTitle && | ||
| !isPage && | ||
| meta.title && | ||
| typeof meta.title === "object" && | ||
| meta.title.template | ||
| ) { | ||
| parentTemplate = meta.title.template; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2133,6 +2133,21 @@ describe("metadata title templates", () => { | |||||||||||||||||||||||||||||||
| expect(result.title).toBe("My Site"); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it("applies ancestor title template to child layout default title", () => { | ||||||||||||||||||||||||||||||||
| // Next.js resolveTitle() applies the stashed ancestor template to title.default: | ||||||||||||||||||||||||||||||||
| // https://github.com/vercel/next.js/blob/canary/packages/next/src/lib/metadata/resolvers/resolve-title.ts | ||||||||||||||||||||||||||||||||
| const result = mergeMetadataEntries([ | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| metadata: { title: { template: "%s | Site", default: "Site" } }, | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
| metadata: { title: { default: "Blog" } }, | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| expect(result.title).toBe("Blog | Site"); | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good test. Consider adding a companion test for the template-only (no default) edge case to lock in the intentional title-clearing behavior:
Suggested change
|
||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it("title.absolute skips all templates", () => { | ||||||||||||||||||||||||||||||||
| const result = mergeMetadata([ | ||||||||||||||||||||||||||||||||
| { title: { template: "%s | My Site", default: "My Site" } }, | ||||||||||||||||||||||||||||||||
|
|
@@ -2150,16 +2165,19 @@ describe("metadata title templates", () => { | |||||||||||||||||||||||||||||||
| expect(result.title).toBe("Hello World - Blog"); | ||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice — this correctly validates that a 3-layer scenario (root template → middle template → page string) applies the nearest template. The fact that the middle layout's |
||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it("page template has no effect (page is terminal)", () => { | ||||||||||||||||||||||||||||||||
| // If the page defines a template, it should be ignored | ||||||||||||||||||||||||||||||||
| // Only layouts define templates, and page is always the last entry | ||||||||||||||||||||||||||||||||
| it("applies ancestor template to page default while ignoring page template", () => { | ||||||||||||||||||||||||||||||||
| const result = mergeMetadata([ | ||||||||||||||||||||||||||||||||
| { title: { template: "%s | Site", default: "Site" } }, | ||||||||||||||||||||||||||||||||
| { title: { template: "%s - Page Template", default: "Page Default" } }, | ||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||
| // The page's template should be ignored; the page's default is used | ||||||||||||||||||||||||||||||||
| // because the page has a title object (not a string), so we use its default | ||||||||||||||||||||||||||||||||
| expect(result.title).toBe("Page Default"); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| expect(result.title).toBe("Page Default | Site"); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it("does not apply a page template to the page's own default title", () => { | ||||||||||||||||||||||||||||||||
| const result = mergeMetadata([{ title: { template: "%s | Page", default: "Page" } }]); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| expect(result.title).toBe("Page"); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| it("preserves non-title metadata during merge", () => { | ||||||||||||||||||||||||||||||||
|
|
@@ -2218,7 +2236,7 @@ describe("metadata title templates", () => { | |||||||||||||||||||||||||||||||
| expect(result).toEqual({ | ||||||||||||||||||||||||||||||||
| description: "Page", | ||||||||||||||||||||||||||||||||
| openGraph: { title: "Slot OG title" }, | ||||||||||||||||||||||||||||||||
| title: "Page", | ||||||||||||||||||||||||||||||||
| title: "Page | Root", | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this uses
/%s/g(global replace), so a template like"%s - %s"would produce"Blog - Blog". The old code used a single.replace("%s", ...). Next.js'sresolveTitleTemplatealso does a single replacement:Actually, checking again — Next.js does use
/%s/gtoo (source). So this matches. Never mind, carry on.