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

[WIP] test Append sitemap extension and optimize imafe metadata static generation #66610

Closed
wants to merge 24 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 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
3 changes: 2 additions & 1 deletion packages/next-swc/crates/next-api/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -968,7 +968,8 @@ impl AppEndpoint {
let manifest_path_prefix = original_name;
let path = node_root
.join(format!("server/app{manifest_path_prefix}/app-paths-manifest.json",).into());
let app_paths_manifest = AppPathsManifest {

let app_paths_manifest: AppPathsManifest = AppPathsManifest {
node_server_app_paths: PagesManifest {
pages: [(original_name.into(), filename)].into_iter().collect(),
},
Expand Down
13 changes: 13 additions & 0 deletions packages/next-swc/crates/next-core/src/app_segment_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ pub struct NextSegmentConfig {
pub runtime: Option<NextRuntime>,
pub preferred_region: Option<Vec<RcStr>>,
pub experimental_ppr: Option<bool>,
/// Wether these metadata exports are defined in the source file.
pub generate_image_metadata: Option<bool>,
pub generate_sitemaps: Option<bool>,
huozhi marked this conversation as resolved.
Show resolved Hide resolved
}

#[turbo_tasks::value_impl]
Expand All @@ -95,6 +98,7 @@ impl NextSegmentConfig {
runtime,
preferred_region,
experimental_ppr,
..
} = self;
*dynamic = dynamic.or(parent.dynamic);
*dynamic_params = dynamic_params.or(parent.dynamic_params);
Expand Down Expand Up @@ -137,6 +141,7 @@ impl NextSegmentConfig {
runtime,
preferred_region,
experimental_ppr,
..
} = self;
merge_parallel(dynamic, &parallel_config.dynamic, "dynamic")?;
merge_parallel(
Expand Down Expand Up @@ -431,6 +436,14 @@ fn parse_config_value(

config.preferred_region = Some(preferred_region);
}
// Match exported generateImageMetadata function and generateSitemaps function, and pass
// them to config.
huozhi marked this conversation as resolved.
Show resolved Hide resolved
"generateImageMetadata" => {
config.generate_image_metadata = Some(true);
}
"generateSitemaps" => {
config.generate_sitemaps = Some(true);
}
huozhi marked this conversation as resolved.
Show resolved Hide resolved
"experimental_ppr" => {
let value = eval_context.eval(init);
let Some(val) = value.as_bool() else {
Expand Down
18 changes: 4 additions & 14 deletions packages/next-swc/crates/next-core/src/next_app/metadata/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub struct MetadataFileMatch<'a> {

fn match_numbered_metadata(stem: &str) -> Option<(&str, &str)> {
let (_whole, stem, number) = lazy_regex::regex_captures!(
"^(icon|apple-icon|opengraph-image|twitter-image)(\\d+)$",
"^(icon|apple-icon|opengraph-image|twitter-image|sitemap)(\\d+)$",
stem
)?;

Expand Down Expand Up @@ -306,8 +306,7 @@ pub fn normalize_metadata_route(mut page: AppPage) -> Result<AppPage> {
route += ".txt"
} else if route == "/manifest" {
route += ".webmanifest"
// Do not append the suffix for the sitemap route
} else if !route.ends_with("/sitemap") {
} else {
// Remove the file extension, e.g. /route-path/robots.txt -> /route-path
let pathname_prefix = split_directory(&route).0.unwrap_or_default();
suffix = get_metadata_route_suffix(pathname_prefix);
Expand All @@ -317,13 +316,8 @@ pub fn normalize_metadata_route(mut page: AppPage) -> Result<AppPage> {
// /<metadata-route>/route.ts. If it's a metadata file route, we need to
// append /[id]/route to the page.
if !route.ends_with("/route") {
let is_static_metadata_file = is_static_metadata_route_file(&page.to_string());
let (base_name, ext) = split_extension(&route);

let is_static_route = route.starts_with("/robots")
|| route.starts_with("/manifest")
|| is_static_metadata_file;

page.0.pop();

page.push(PageSegment::Static(
Expand All @@ -338,10 +332,6 @@ pub fn normalize_metadata_route(mut page: AppPage) -> Result<AppPage> {
.into(),
))?;

if !is_static_route {
page.push(PageSegment::OptionalCatchAll("__metadata_id__".into()))?;
}

page.push(PageSegment::PageType(PageType::Route))?;
}

Expand All @@ -358,11 +348,11 @@ mod test {
let cases = vec![
[
"/client/(meme)/more-route/twitter-image",
"/client/(meme)/more-route/twitter-image-769mad/[[...__metadata_id__]]/route",
"/client/(meme)/more-route/twitter-image-769mad/route",
],
[
"/client/(meme)/more-route/twitter-image2",
"/client/(meme)/more-route/twitter-image2-769mad/[[...__metadata_id__]]/route",
"/client/(meme)/more-route/twitter-image2-769mad/route",
],
["/robots.txt", "/robots.txt/route"],
["/manifest.webmanifest", "/manifest.webmanifest/route"],
Expand Down
91 changes: 57 additions & 34 deletions packages/next-swc/crates/next-core/src/next_app/metadata/route.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//!
//! See `next/src/build/webpack/loaders/next-metadata-route-loader`

use anyhow::{bail, Result};
use anyhow::{bail, Ok, Result};
use base64::{display::Base64Display, engine::general_purpose::STANDARD};
use indoc::{formatdoc, indoc};
use turbo_tasks::{ValueToString, Vc};
Expand All @@ -22,7 +22,9 @@ use super::get_content_type;
use crate::{
app_structure::MetadataItem,
mode::NextMode,
next_app::{app_entry::AppEntry, app_route_entry::get_app_route_entry, AppPage, PageSegment},
next_app::{
app_entry::AppEntry, app_route_entry::get_app_route_entry, AppPage, PageSegment, PageType,
},
next_config::NextConfig,
parse_segment_config_from_source,
};
Expand Down Expand Up @@ -52,11 +54,11 @@ pub async fn get_app_metadata_route_source(
}

#[turbo_tasks::function]
pub fn get_app_metadata_route_entry(
pub async fn get_app_metadata_route_entry(
nodejs_context: Vc<ModuleAssetContext>,
edge_context: Vc<ModuleAssetContext>,
project_root: Vc<FileSystemPath>,
page: AppPage,
mut page: AppPage,
mode: NextMode,
metadata: MetadataItem,
next_config: Vc<NextConfig>,
Expand All @@ -70,11 +72,38 @@ pub fn get_app_metadata_route_entry(
let source = Vc::upcast(FileSource::new(original_path));
let segment_config = parse_segment_config_from_source(source);

// is_multi_dynamic is true when config.generateSitemaps or
// config.generateImageMetadata is defined
let is_multi_dynamic: bool = if Some(segment_config).is_some() {
let config = segment_config.await.unwrap();
config.generate_sitemaps.is_some() || config.generate_image_metadata.is_some()
} else {
false
};

// remove the last /route segment of page
page.0.pop();

let _ = if is_multi_dynamic {
// push /[__metadata_id__] to the page
page.push(PageSegment::Dynamic("__metadata_id__".into()))
} else {
// if page last segment is sitemap, change to sitemap.xml
if page.last() == Some(&PageSegment::Static("sitemap".into())) {
page.0.pop();
page.push(PageSegment::Static("sitemap.xml".into()))
} else {
Ok(())
}
};
// Push /route back
let _ = page.push(PageSegment::PageType(PageType::Route));

get_app_route_entry(
nodejs_context,
edge_context,
get_app_metadata_route_source(page.clone(), mode, metadata),
page,
page.clone(),
project_root,
Some(segment_config),
next_config,
Expand Down Expand Up @@ -213,21 +242,19 @@ async fn dynamic_site_map_route_source(
let stem = path.file_stem().await?;
let stem = stem.as_deref().unwrap_or_default();
let ext = &*path.extension().await?;

let content_type = get_content_type(path).await?;

let mut static_generation_code = "";

if mode.is_production() && page.contains(&PageSegment::Dynamic("[__metadata_id__]".into())) {
if mode.is_production() && page.contains(&PageSegment::Dynamic("__metadata_id__".into())) {
static_generation_code = indoc! {
r#"
export async function generateStaticParams() {
const sitemaps = await generateSitemaps()
const params = []

for (const item of sitemaps) {
params.push({ __metadata_id__: item.id.toString() })
}
for (const item of sitemaps) {{
params.push({ __metadata_id__: item.id.toString() + '.xml' })
}}
Comment on lines +255 to +257
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const item of sitemaps) {{
params.push({ __metadata_id__: item.id.toString() + '.xml' })
}}
for (const item of sitemaps) {
params.push({ __metadata_id__: item.id.toString() + '.xml' })
}

That's not in a format! function

Copy link
Member Author

Choose a reason for hiding this comment

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

The r# requires to have it otherwise it errors

error: invalid format string: expected `'}'`, found `'i'`
   --> packages/next-swc/crates/next-core/src/next_app/metadata/route.rs:287:3
    |
286 |                         status: 404,
    |                          - because of this opening brace
287 |                     }})
    |   ^ expected `'}'` in format string
    |
    = note: if you intended to print `{`, you can escape it using `{{`

return params
}
"#,
Expand All @@ -252,29 +279,25 @@ async fn dynamic_site_map_route_source(
}}

export async function GET(_, ctx) {{
const {{ __metadata_id__ = [], ...params }} = ctx.params || {{}}
const targetId = __metadata_id__[0]
let id = undefined
const sitemaps = generateSitemaps ? await generateSitemaps() : null
const {{ __metadata_id__: id, ...params }} = ctx.params || {{}}
const hasXmlExtension = id ? id.endsWith('.xml') : false
if (id && !hasXmlExtension) {{
return new NextResponse('Not Found', {{
status: 404,
}})
}}

if (sitemaps) {{
id = sitemaps.find((item) => {{
if (process.env.NODE_ENV !== 'production') {{
if (item?.id == null) {{
throw new Error('id property is required for every item returned from generateSitemaps')
}}
if (process.env.NODE_ENV !== 'production' && sitemapModule.generateSitemaps) {{
const sitemaps = await sitemapModule.generateSitemaps()
for (const item of sitemaps) {{
if (item?.id == null) {{
throw new Error('id property is required for every item returned from generateSitemaps')
}}
return item.id.toString() === targetId
}})?.id

if (id == null) {{
return new NextResponse('Not Found', {{
status: 404,
}})
}}
}}

const data = await handler({{ id }})

const targetId = id && hasXmlExtension ? id.slice(0, -4) : undefined
const data = await handler({{ id: targetId }})
const content = resolveRouteData(data, fileType)

return new NextResponse(content, {{
Expand Down Expand Up @@ -324,12 +347,12 @@ async fn dynamic_image_route_source(path: Vc<FileSystemPath>) -> Result<Vc<Box<d
}}

export async function GET(_, ctx) {{
const {{ __metadata_id__ = [], ...params }} = ctx.params || {{}}
const targetId = __metadata_id__[0]
const {{ __metadata_id__, ...params }} = ctx.params || {{}}
const targetId = __metadata_id__
let id = undefined
const imageMetadata = generateImageMetadata ? await generateImageMetadata({{ params }}) : null

if (imageMetadata) {{
if (generateImageMetadata) {{
const imageMetadata = await generateImageMetadata({{ params }})
id = imageMetadata.find((item) => {{
if (process.env.NODE_ENV !== 'production') {{
if (item?.id == null) {{
Expand Down
28 changes: 11 additions & 17 deletions packages/next/src/build/analysis/get-page-static-info.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export interface PageStaticInfo {
ssr?: boolean
rsc?: RSCModuleType
generateStaticParams?: boolean
generateSitemaps?: boolean
generateImageMetadata?: boolean
middleware?: MiddlewareConfigParsed
amp?: boolean | 'hybrid'
extraConfig?: Record<string, any>
Expand Down Expand Up @@ -141,8 +143,8 @@ function checkExports(
ssg: boolean
runtime?: string
preferredRegion?: string | string[]
generateImageMetadata?: boolean
generateSitemaps?: boolean
generateImageMetadata: boolean
generateSitemaps: boolean
generateStaticParams: boolean
extraProperties?: Set<string>
directives?: Set<string>
Expand Down Expand Up @@ -467,20 +469,6 @@ function warnAboutUnsupportedValue(
warnedUnsupportedValueMap.set(pageFilePath, true)
}

// Detect if metadata routes is a dynamic route, which containing
// generateImageMetadata or generateSitemaps as export
export async function isDynamicMetadataRoute(
pageFilePath: string
): Promise<boolean> {
const fileContent = (await tryToReadFile(pageFilePath, true)) || ''
if (/generateImageMetadata|generateSitemaps/.test(fileContent)) {
const swcAST = await parseModule(pageFilePath, fileContent)
const exportsInfo = checkExports(swcAST, pageFilePath)
return !!(exportsInfo.generateImageMetadata || exportsInfo.generateSitemaps)
}
return false
}

/**
* For a given pageFilePath and nextConfig, if the config supports it, this
* function will read the file and return the runtime that should be used.
Expand All @@ -499,7 +487,7 @@ export async function getPageStaticInfo(params: {

const fileContent = (await tryToReadFile(pageFilePath, !isDev)) || ''
if (
/(?<!(_jsx|jsx-))runtime|preferredRegion|getStaticProps|getServerSideProps|generateStaticParams|export const/.test(
/(?<!(_jsx|jsx-))runtime|preferredRegion|getStaticProps|getServerSideProps|generateStaticParams|export const|generateImageMetadata|generateSitemaps/.test(
fileContent
)
) {
Expand All @@ -510,6 +498,8 @@ export async function getPageStaticInfo(params: {
runtime,
preferredRegion,
generateStaticParams,
generateImageMetadata,
generateSitemaps,
extraProperties,
directives,
} = checkExports(swcAST, pageFilePath)
Expand Down Expand Up @@ -651,6 +641,8 @@ export async function getPageStaticInfo(params: {
ssg,
rsc,
generateStaticParams,
generateImageMetadata,
generateSitemaps,
amp: config.amp || false,
...(middlewareConfig && { middleware: middlewareConfig }),
...(resolvedRuntime && { runtime: resolvedRuntime }),
Expand All @@ -664,6 +656,8 @@ export async function getPageStaticInfo(params: {
ssg: false,
rsc: RSC_MODULE_TYPES.server,
generateStaticParams: false,
generateImageMetadata: false,
generateSitemaps: false,
amp: false,
runtime: undefined,
}
Expand Down
Loading
Loading