Skip to content

Commit

Permalink
Remove auto appending xml extension to dynamic sitemap routes (#65507)
Browse files Browse the repository at this point in the history
### What

Remove the auto appending `.xml` extension to the sitemap routes when
it's a dynamic route.


### Why

Previously we were adding `.xml` to `/[...paths/]sitemap` routes, but
the bad part is when you use it to generate multiple sitemaps with
`generateSitemaps` in format like `/[...paths/]sitemap.xml/[id]`, which
doesn't look good in url format and it can be inferred as xml with
content-type. Hence we don't need to add `.xml` in the url.

Before this change it could also result into the different url between
dev and prod:
dev: `/sitemap.xml/[id]`
prod: `/sitemap/[id].xml` 

Now it's going to be aligned as `/sitemap/[id]`. Users can add extension
flexiblely.

Closes NEXT-3357
  • Loading branch information
huozhi committed May 9, 2024
1 parent 1277ff4 commit 09baadf
Show file tree
Hide file tree
Showing 10 changed files with 115 additions and 117 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,7 @@ export default async function sitemap({ id }) {
}
```

In production, your generated sitemaps will be available at `/.../sitemap/[id].xml`. For example, `/product/sitemap/1.xml`.

In development, you can view the generated sitemap on `/.../sitemap.xml/[id]`. For example, `/product/sitemap.xml/1`. This difference is temporary and will follow the production format.
Your generated sitemaps will be available at `/.../sitemap/[id]`. For example, `/product/sitemap/1`.

See the [`generateSitemaps` API reference](/docs/app/api-reference/functions/generate-sitemaps) for more information.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,8 @@ pub fn normalize_metadata_route(mut page: AppPage) -> Result<AppPage> {
route += ".txt"
} else if route == "/manifest" {
route += ".webmanifest"
} else if route.ends_with("/sitemap") {
route += ".xml"
} else {
// Do not append the suffix for the sitemap route
} else if !route.ends_with("/sitemap") {
// 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 Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ async fn dynamic_site_map_route_source(
const params = []
for (const item of sitemaps) {
params.push({ __metadata_id__: item.id.toString() + '.xml' })
params.push({ __metadata_id__: item.id.toString() })
}
return params
}
Expand Down
4 changes: 2 additions & 2 deletions packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -998,13 +998,13 @@ export default async function build(
}

if (
pageKey.includes('sitemap.xml/[[...__metadata_id__]]') &&
pageKey.includes('sitemap/[[...__metadata_id__]]') &&
isDynamic
) {
delete mappedAppPages[pageKey]
mappedAppPages[
pageKey.replace(
'sitemap.xml/[[...__metadata_id__]]',
'sitemap/[[...__metadata_id__]]',
'sitemap/[__metadata_id__]'
)
] = pagePath
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export async function generateStaticParams() {
const params = []
for (const item of sitemaps) {
params.push({ __metadata_id__: item.id.toString() + '.xml' })
params.push({ __metadata_id__: item.id.toString() })
}
return params
}
Expand Down Expand Up @@ -225,9 +225,6 @@ export async function GET(_, ctx) {
}
}
let itemID = item.id.toString()
if(process.env.NODE_ENV === 'production') {
itemID += '.xml'
}
return itemID === targetId
})?.id
if (id == null) {
Expand Down
6 changes: 3 additions & 3 deletions packages/next/src/lib/metadata/get-metadata-route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ export function normalizeMetadataRoute(page: string) {
route += '.txt'
} else if (page === '/manifest') {
route += '.webmanifest'
} else if (page.endsWith('/sitemap')) {
route += '.xml'
} else {
}
// For sitemap, we don't automatically add the route suffix since it can have sub-routes
else if (!page.endsWith('/sitemap')) {
// Remove the file extension, e.g. /route-path/robots.txt -> /route-path
const pathnamePrefix = page.slice(0, -(path.basename(page).length + 1))
suffix = getMetadataRouteSuffix(pathnamePrefix)
Expand Down
35 changes: 18 additions & 17 deletions test/e2e/app-dir/dynamic-in-generate-params/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,40 @@
import { type NextInstance, nextTestSetup } from 'e2e-utils'
import type { Response } from 'node-fetch'

async function getLastModifiedTime(next: NextInstance, pathname: string) {
const content = await (await next.fetch(pathname)).text()
return content.match(/<lastmod>([^<]+)<\/lastmod>/)[1]
}

function assertSitemapResponse(res: Response) {
expect(res.status).toBe(200)
expect(res.headers.get('content-type')).toContain('application/xml')
}

describe('app-dir - dynamic in generate params', () => {
const { next, isNextDev } = nextTestSetup({
const { next } = nextTestSetup({
files: __dirname,
skipDeployment: true,
})

it('should render sitemap with generateSitemaps in force-dynamic config dynamically', async () => {
const firstTime = await getLastModifiedTime(
next,
isNextDev ? 'sitemap.xml/0' : '/sitemap/0.xml'
)
const secondTime = await getLastModifiedTime(
next,
isNextDev ? 'sitemap.xml/0' : '/sitemap/0.xml'
)
const firstTime = await getLastModifiedTime(next, 'sitemap/0')
const secondTime = await getLastModifiedTime(next, 'sitemap/0')

expect(firstTime).not.toEqual(secondTime)
})

it('should be able to call while generating multiple dynamic sitemaps', async () => {
expect(
(await next.fetch(isNextDev ? 'sitemap.xml/0' : '/sitemap/0.xml')).status
).toBe(200)
expect(
(await next.fetch(isNextDev ? 'sitemap.xml/1' : '/sitemap/1.xml')).status
).toBe(200)
const res0 = await next.fetch('sitemap/0')
const res1 = await next.fetch('sitemap/1')
assertSitemapResponse(res0)
assertSitemapResponse(res1)
})

it('should be able to call fetch while generating multiple dynamic pages', async () => {
expect((await next.fetch('/dynamic/0')).status).toBe(200)
expect((await next.fetch('/dynamic/1')).status).toBe(200)
const pageRes0 = await next.fetch('dynamic/0')
const pageRes1 = await next.fetch('dynamic/1')
expect(pageRes0.status).toBe(200)
expect(pageRes1.status).toBe(200)
})
})
133 changes: 66 additions & 67 deletions test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe('app dir - metadata dynamic routes', () => {
},
})

describe('text routes', () => {
describe('robots.txt', () => {
it('should handle robots.[ext] dynamic routes', async () => {
const res = await next.fetch('/robots.txt')
const text = await res.text()
Expand All @@ -40,39 +40,53 @@ describe('app dir - metadata dynamic routes', () => {
"
`)
})
})

describe('sitemap', () => {
it('should handle sitemap.[ext] dynamic routes', async () => {
const res = await next.fetch('/sitemap.xml')
const res = await next.fetch('/sitemap')
const text = await res.text()

expect(res.headers.get('content-type')).toBe('application/xml')
expect(res.headers.get('cache-control')).toBe(CACHE_HEADERS.REVALIDATE)

expect(text).toMatchInlineSnapshot(`
"<?xml version="1.0" encoding="UTF-8"?>
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
<url>
<loc>https://example.com</loc>
<lastmod>2021-01-01</lastmod>
<changefreq>weekly</changefreq>
<priority>0.5</priority>
</url>
<url>
<loc>https://example.com/about</loc>
<lastmod>2021-01-01</lastmod>
</url>
</urlset>
"
`)
"<?xml version="1.0" encoding="UTF-8"?>
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
<url>
<loc>https://example.com</loc>
<lastmod>2021-01-01</lastmod>
<changefreq>weekly</changefreq>
<priority>0.5</priority>
</url>
<url>
<loc>https://example.com/about</loc>
<lastmod>2021-01-01</lastmod>
</url>
</urlset>
"
`)
})

it('should support generate multi sitemaps with generateSitemaps', async () => {
const ids = ['child0', 'child1', 'child2', 'child3']
function fetchSitemap(id) {
return next.fetch(`/gsp/sitemap/${id}`).then((res) => res.text())
}

for (const id of ids) {
const text = await fetchSitemap(id)
expect(text).toContain(`<loc>https://example.com/dynamic/${id}</loc>`)
}
})

it('should not throw if client components are imported but not used', async () => {
const { status } = await next.fetch('/client-ref-dependency/sitemap.xml')
it('should not throw if client components are imported but not used in sitemap', async () => {
const { status } = await next.fetch('/client-ref-dependency/sitemap')
expect(status).toBe(200)
})

it('should support alternate.languages in sitemap', async () => {
const xml = await (await next.fetch('/lang/sitemap.xml')).text()
const xml = await (await next.fetch('/lang/sitemap')).text()

expect(xml).toContain('xmlns:xhtml="http://www.w3.org/1999/xhtml')
expect(xml).toContain(
Expand All @@ -82,6 +96,35 @@ describe('app dir - metadata dynamic routes', () => {
`<xhtml:link rel="alternate" hreflang="de" href="https://example.com/de/about" />`
)
})
if (isNextStart) {
it('should optimize routes without multiple generation API as static routes', async () => {
const appPathsManifest = JSON.parse(
await next.readFile('.next/server/app-paths-manifest.json')
)

expect(appPathsManifest).toMatchObject({
// static routes
'/twitter-image/route': 'app/twitter-image/route.js',
'/sitemap/route': 'app/sitemap/route.js',

// dynamic
'/gsp/sitemap/[__metadata_id__]/route':
'app/gsp/sitemap/[__metadata_id__]/route.js',
'/(group)/dynamic/[size]/apple-icon-ahg52g/[[...__metadata_id__]]/route':
'app/(group)/dynamic/[size]/apple-icon-ahg52g/[[...__metadata_id__]]/route.js',
})
})

it('should generate static paths of dynamic sitemap in production', async () => {
const sitemapPaths = ['child0', 'child1', 'child2', 'child3'].map(
(id) => `.next/server/app/gsp/sitemap/${id}.meta`
)
const promises = sitemapPaths.map(async (filePath) => {
expect(await next.hasFile(filePath)).toBe(true)
})
await Promise.all(promises)
})
}
})

describe('social image routes', () => {
Expand Down Expand Up @@ -203,22 +246,6 @@ describe('app dir - metadata dynamic routes', () => {
])
})

it('should support generate multi sitemaps with generateSitemaps', async () => {
const ids = ['child0', 'child1', 'child2', 'child3']
function fetchSitemap(id) {
return next
.fetch(
isNextDev ? `/gsp/sitemap.xml/${id}` : `/gsp/sitemap/${id}.xml`
)
.then((res) => res.text())
}

for (const id of ids) {
const text = await fetchSitemap(id)
expect(text).toContain(`<loc>https://example.com/dynamic/${id}</loc>`)
}
})

it('should fill params into dynamic routes url of metadata images', async () => {
const $ = await next.render$('/dynamic/big')
const ogImageUrl = $('meta[property="og:image"]').attr('content')
Expand Down Expand Up @@ -289,7 +316,7 @@ describe('app dir - metadata dynamic routes', () => {
if (isNextStart) {
describe('route segment config', () => {
it('should generate dynamic route if dynamic config is force-dynamic', async () => {
const dynamicRoute = '/route-config/sitemap.xml'
const dynamicRoute = '/route-config/sitemap'

expect(
await next.hasFile(`.next/server/app${dynamicRoute}/route.js`)
Expand Down Expand Up @@ -480,7 +507,7 @@ describe('app dir - metadata dynamic routes', () => {
const outputBeforeFetch = next.cliOutput + ''

await next.patchFile(sitemapFilePath, contentMissingIdProperty)
await next.fetch('/metadata-base/unset/sitemap.xml/0')
await next.fetch('/metadata-base/unset/sitemap/0')

const outputAfterFetch = next.cliOutput + ''
const output = outputAfterFetch.replace(outputBeforeFetch, '')
Expand All @@ -494,7 +521,7 @@ describe('app dir - metadata dynamic routes', () => {
}, /success/)
} finally {
await next.deleteFile(sitemapFilePath)
await next.fetch('/metadata-base/unset/sitemap.xml/0')
await next.fetch('/metadata-base/unset/sitemap/0')
}
})

Expand Down Expand Up @@ -537,34 +564,6 @@ describe('app dir - metadata dynamic routes', () => {
expect(edgeRoute).toMatch(/\/\(group\)\/twitter-image-\w{6}\/route/)
})

it('should optimize routes without multiple generation API as static routes', async () => {
const appPathsManifest = JSON.parse(
await next.readFile('.next/server/app-paths-manifest.json')
)

expect(appPathsManifest).toMatchObject({
// static routes
'/twitter-image/route': 'app/twitter-image/route.js',
'/sitemap.xml/route': 'app/sitemap.xml/route.js',

// dynamic
'/gsp/sitemap/[__metadata_id__]/route':
'app/gsp/sitemap/[__metadata_id__]/route.js',
'/(group)/dynamic/[size]/apple-icon-ahg52g/[[...__metadata_id__]]/route':
'app/(group)/dynamic/[size]/apple-icon-ahg52g/[[...__metadata_id__]]/route.js',
})
})

it('should generate static paths of dynamic sitemap in production', async () => {
const sitemapPaths = ['child0', 'child1', 'child2', 'child3'].map(
(id) => `.next/server/app/gsp/sitemap/${id}.xml.meta`
)
const promises = sitemapPaths.map(async (filePath) => {
expect(await next.hasFile(filePath)).toBe(true)
})
await Promise.all(promises)
})

it('should include default og font files in file trace', async () => {
const fileTrace = JSON.parse(
await next.readFile(
Expand Down
26 changes: 15 additions & 11 deletions test/turbopack-build-tests-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -1871,28 +1871,32 @@
"failed": [
"app dir - metadata dynamic routes icon image routes should render apple icon with dynamic routes",
"app dir - metadata dynamic routes icon image routes should render icon with dynamic routes",
"app dir - metadata dynamic routes route segment config should generate dynamic route if dynamic config is force-dynamic",
"app dir - metadata dynamic routes should generate static paths of dynamic sitemap in production",
"app dir - metadata dynamic routes should error if the default export of dynamic image is missing",
"app dir - metadata dynamic routes should error when id is missing in generateImageMetadata",
"app dir - metadata dynamic routes should error when id is missing in generateSitemaps",
"app dir - metadata dynamic routes should generate unique path for image routes under group routes",
"app dir - metadata dynamic routes should include default og font files in file trace",
"app dir - metadata dynamic routes should inject dynamic metadata properly to head",
"app dir - metadata dynamic routes should optimize routes without multiple generation API as static routes",
"app dir - metadata dynamic routes should pick configured metadataBase instead of deployment url for canonical url",
"app dir - metadata dynamic routes should support edge runtime of image routes",
"app dir - metadata dynamic routes should use localhost for local prod and fallback to deployment url when metadataBase is falsy",
"app dir - metadata dynamic routes social image routes should fill params into dynamic routes url of metadata images",
"app dir - metadata dynamic routes social image routes should fill params into routes groups url of static images",
"app dir - metadata dynamic routes social image routes should handle custom fonts in both edge and nodejs runtime",
"app dir - metadata dynamic routes social image routes should handle manifest.[ext] dynamic routes",
"app dir - metadata dynamic routes social image routes should render og image with opengraph-image dynamic routes",
"app dir - metadata dynamic routes social image routes should render og image with twitter-image dynamic routes",
"app dir - metadata dynamic routes social image routes should support generate multi images with generateImageMetadata",
"app dir - metadata dynamic routes social image routes should support generate multi sitemaps with generateSitemaps",
"app dir - metadata dynamic routes social image routes should support params as argument in dynamic routes",
"app dir - metadata dynamic routes text routes should handle robots.[ext] dynamic routes",
"app dir - metadata dynamic routes text routes should handle sitemap.[ext] dynamic routes",
"app dir - metadata dynamic routes text routes should not throw if client components are imported but not used",
"app dir - metadata dynamic routes text routes should support alternate.languages in sitemap"
"app dir - metadata dynamic routes social image routes should support generate multi images with generateImageMetadata",
"app dir - metadata dynamic routes robots.txt should handle robots.[ext] dynamic routes",
"app dir - metadata dynamic routes should support edge runtime of image routes",
"app dir - metadata dynamic routes should include default og font files in file trace",
"app dir - metadata dynamic routes route segment config should generate dynamic route if dynamic config is force-dynamic",
"app dir - metadata dynamic routes sitemap should not throw if client components are imported but not used in sitemap",
"app dir - metadata dynamic routes sitemap should handle sitemap.[ext] dynamic routes",
"app dir - metadata dynamic routes sitemap social image routes should support generate multi sitemaps with generateSitemaps",
"app dir - metadata dynamic routes sitemap should support alternate.languages in sitemap",
"app dir - metadata dynamic routes sitemap should support generate multi sitemaps with generateSitemaps",
"app dir - metadata dynamic routes sitemap should optimize routes without multiple generation API as static routes",
"app dir - metadata dynamic routes sitemap should generate static paths of dynamic sitemap in production"
],
"pending": [],
"flakey": [],
Expand Down

0 comments on commit 09baadf

Please sign in to comment.