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.

Sign up for GitHub

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(metadata): align metadata suffix hash between turbopack #57544

Merged
merged 2 commits into from
Oct 30, 2023
Merged

fix(metadata): align metadata suffix hash between turbopack #57544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b0cf6b5
fix(metadata): align metadata suffix hash between turbopack
kwonoj Oct 26, 2023
e9cc837
Merge branch 'canary' into metadata-dynamic
kodiakhq[bot] Oct 30, 2023
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
5 changes: 3 additions & 2 deletions packages/next-swc/crates/next-core/src/next_app/metadata/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ fn djb2_hash(str: &str) -> u32 {
})
}

// this is here to mirror next.js behaviour.
// this is here to mirror next.js behaviour (`toString(36).slice(0, 6)`)
fn format_radix(mut x: u32, radix: u32) -> String {
let mut result = vec![];

Expand All @@ -273,7 +273,8 @@ fn format_radix(mut x: u32, radix: u32) -> String {
}
}

result.into_iter().rev().collect()
result.reverse();
result[..6].iter().collect()
Comment on lines +276 to +277
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think you can

Suggested change
result.reverse();
result[..6].iter().collect()
result.into_iter().rev().take(6).collect()

Copy link
Contributor

Choose a reason for hiding this comment

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

this should happen in get_metadata_route_suffix below, not in here

}

/// If there's special convention like (...) or @ in the page path,
Expand Down
10 changes: 8 additions & 2 deletions packages/next/src/shared/lib/hash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
// http://www.cse.yorku.ca/~oz/hash.html
// More specifically, 32-bit hash via djbxor
// (ref: https://gist.github.com/eplawless/52813b1d8ad9af510d85?permalink_comment_id=3367765#gistcomment-3367765)
// This is due to number type differences between rust for turbopack to js number types,
// where rust does not have easy way to repreesnt js's 53-bit float number type for the matching
// overflow behavior. This is more `correct` in terms of having canonical hash across different runtime / implementation
// as can gaurantee determinstic output from 32bit hash.
export function djb2Hash(str: string) {
let hash = 5381
for (let i = 0; i < str.length; i++) {
const char = str.charCodeAt(i)
hash = (hash << 5) + hash + char
hash = ((hash << 5) + hash + char) & 0xffffffff
}
return Math.abs(hash)
return hash >>> 0
}

export function hexHash(str: string) {
Expand Down
14 changes: 7 additions & 7 deletions test/e2e/app-dir/metadata-dynamic-routes/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const CACHE_HEADERS = {
REVALIDATE: 'public, max-age=0, must-revalidate',
}

const hashRegex = /\?\w+$/
const hashRegex = /\?\w+/

createNextDescribe(
'app dir - metadata dynamic routes',
Expand Down Expand Up @@ -160,12 +160,12 @@ createNextDescribe(
// slug is id param from generateImageMetadata
expect(iconUrls).toMatchObject([
{
href: '/dynamic/big/icon-48jo90/small',
href: '/dynamic/big/icon-ahg52g/small',
sizes: '48x48',
type: 'image/png',
},
{
href: '/dynamic/big/icon-48jo90/medium',
href: '/dynamic/big/icon-ahg52g/medium',
sizes: '72x72',
type: 'image/png',
},
Expand All @@ -183,12 +183,12 @@ createNextDescribe(
// slug is index by default
expect(appleTouchIconUrls).toEqual([
{
href: '/dynamic/big/apple-icon-48jo90/0',
href: '/dynamic/big/apple-icon-ahg52g/0',
sizes: '48x48',
type: 'image/png',
},
{
href: '/dynamic/big/apple-icon-48jo90/1',
href: '/dynamic/big/apple-icon-ahg52g/1',
sizes: '64x64',
type: 'image/png',
},
Expand Down Expand Up @@ -551,8 +551,8 @@ createNextDescribe(
// dynamic
'/gsp/sitemap/[__metadata_id__]/route':
'app/gsp/sitemap/[__metadata_id__]/route.js',
'/(group)/dynamic/[size]/apple-icon-48jo90/[[...__metadata_id__]]/route':
'app/(group)/dynamic/[size]/apple-icon-48jo90/[[...__metadata_id__]]/route.js',
'/(group)/dynamic/[size]/apple-icon-ahg52g/[[...__metadata_id__]]/route':
'app/(group)/dynamic/[size]/apple-icon-ahg52g/[[...__metadata_id__]]/route.js',
})
})

Expand Down
10 changes: 5 additions & 5 deletions test/turbopack-tests-manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3314,13 +3314,13 @@
"app dir - metadata dynamic routes social image routes should support generate multi sitemaps with generateSitemaps",
"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 not throw if client components are imported but not used",
"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 support params as argument in dynamic routes",
"app dir - metadata dynamic routes should generate unique path for image routes under group routes"
],
"failed": [
"app dir - metadata dynamic routes should generate unique path for image routes under group routes",
"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 support params as argument in dynamic routes"
"app dir - metadata dynamic routes social image routes should handle custom fonts in both edge and nodejs runtime"
],
"pending": [],
"flakey": [],
Expand Down
Loading