-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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(loader_tree): propagate metadata to corresponding layout #56956
Conversation
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
|
f6d12eb
to
918230e
Compare
Tests Passed |
72ddb5c
to
dab41aa
Compare
@@ -671,7 +671,7 @@ createNextDescribe( | |||
it('should support root level of favicon.ico', async () => { | |||
let $ = await next.render$('/') | |||
const favIcon = $('link[rel="icon"]') | |||
expect(favIcon.attr('href')).toBe('/favicon.ico') | |||
expect(favIcon.attr('href')).toMatch('/favicon.ico') |
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.
what will the full url of /favicon.ico
? we expect /favicon.ico
is the fixed one for backcompatible
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.
Turbopack currently puts a querystring after favicon.ico with its hashes. So if file's the same url would be probably remain as-is.
dab41aa
to
13529ea
Compare
13529ea
to
f243acd
Compare
let components = if components.metadata.base_page.is_some() { | ||
components | ||
} else { | ||
(Components { | ||
metadata: Metadata { | ||
base_page: Some(app_page.clone()), | ||
..components.metadata.clone() | ||
}, | ||
..*components | ||
}) | ||
.cell() | ||
.await? | ||
}; |
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.
let components = if components.metadata.base_page.is_some() { | |
components | |
} else { | |
(Components { | |
metadata: Metadata { | |
base_page: Some(app_page.clone()), | |
..components.metadata.clone() | |
}, | |
..*components | |
}) | |
.cell() | |
.await? | |
}; | |
let components = if components.metadata.base_page.is_some() { | |
Cow::Borrowed(&components) | |
} else { | |
Cow::Owned(Components { | |
metadata: Metadata { | |
base_page: Some(app_page.clone()), | |
..components.metadata.clone() | |
}, | |
..*components | |
}) | |
}; |
No need to cell and read it
What?
next.js/test/e2e/app-dir/metadata/metadata.test.ts
Line 487 in 17553c5
The way next.js collects static metadata is read static metadata, and then read layout metadata to merge multiple metadatas into a single layout path (
next.js/packages/next/src/lib/metadata/resolve-metadata.ts
Lines 347 to 352 in 17553c5
When turbopack creates LoaderTree for the corresponding directory tree, it extracts
page
but skips metadata in result there are orphan components that have a metadata doesn't have layout metadata, as well as a component have a layout doesn't have metadata. Latter is being rendered as a page (since it have correct layout), which eventually falls back to the default metadata instead.PR trickles down the metadata when extracting page (creating a new component with
page
) to consolidates those.Also PR expands Metadata to have base_page property to capture where it has been originally exists, as we clone down metadata then do
fillMetadataSegment
against the current page where LoaderTree is being created it creates a wrong relative path. For example, currentlyWhen recursively traverse directory tree, capture each components with corresponding base_page to calculate instead.
Unfortunately this doesn't make pass all of the metadata tests; there are lot to dig more. Would like to scope PR in a reasonable size.
Closes WEB-1795