Skip to content

Commit

Permalink
fix(metadata): align metadata suffix hash between turbopack (#57544)
Browse files Browse the repository at this point in the history
### What?

Wraps up metadata-dynamic-routes tests fixes for the turbopack. There is 1 remaining failing test due to lacks of supporting `import.meta.url` which need to be addressed separately.

I spent amount of time why turbopack cannot find the route for the dynamic metadata for a certain route. In the end, found there are mismatching expectations for the route due to different hash for the certain route. We do use the same djb2 hash between next.js and turbopack both, so it was quite confusing why we don't get deterministic hash.

After trying some experiments, found out root cause was how 2 different runtimes handle overflow for given type of numbers. In rust + turbpack we use u32 and do 32-bit hash calculation for given string, while in js we implicitly used number type as is, in result overflow occurs with default 53-bit float. 

Originally I tried to adjust hash in turbopack side to preserve js hash as-is, but so far I found it was non trivial to do so as rust there's no out of the box types we can coerce to the js number type. In result, unlike other fixes in turbopack this PR changes how js hash is being generated. I hope this woulndn't be a breaking changes; expect so since this is a metadata specific hash that we do not have written spec for it.

Closes WEB-1890
  • Loading branch information
kwonoj authored Oct 30, 2023
1 parent 4df888a commit cce9f0d
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 16 deletions.
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()
}

/// 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
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
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
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

0 comments on commit cce9f0d

Please sign in to comment.