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

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

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented Oct 26, 2023

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

@ijjk ijjk added area: tests Turbopack Related to Turbopack with Next.js. created-by: Turbopack team PRs by the Turbopack team. type: next labels Oct 26, 2023
@kwonoj kwonoj changed the title fix(metadata): align metadata suffix hash between turbopack [WIP] fix(metadata): align metadata suffix hash between turbopack Oct 26, 2023
@ijjk
Copy link
Member

ijjk commented Oct 26, 2023

Tests Passed

@ijjk
Copy link
Member

ijjk commented Oct 26, 2023

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js metadata-dynamic Change
buildDuration 10.4s 10.4s N/A
buildDurationCached 5.9s 6s N/A
nodeModulesSize 175 MB 175 MB ⚠️ +2.36 kB
nextStartRea..uration (ms) 416ms 422ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js metadata-dynamic Change
199-HASH.js gzip 30 kB 30 kB N/A
3f784ff6-HASH.js gzip 53.2 kB 53.2 kB
99.HASH.js gzip 182 B 182 B
framework-HASH.js gzip 45.5 kB 45.5 kB
main-app-HASH.js gzip 254 B 252 B N/A
main-HASH.js gzip 33.1 kB 33.1 kB N/A
webpack-HASH.js gzip 1.75 kB 1.75 kB N/A
Overall change 98.9 kB 98.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js metadata-dynamic Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js metadata-dynamic Change
_app-HASH.js gzip 206 B 205 B N/A
_error-HASH.js gzip 182 B 180 B N/A
amp-HASH.js gzip 506 B 505 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.59 kB 2.59 kB
edge-ssr-HASH.js gzip 260 B 259 B N/A
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 369 B 369 B
image-HASH.js gzip 4.38 kB 4.38 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.67 kB 2.67 kB N/A
routerDirect..HASH.js gzip 316 B 318 B N/A
script-HASH.js gzip 385 B 384 B N/A
withRouter-HASH.js gzip 319 B 319 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.99 kB 3.99 kB
Client Build Manifests
vercel/next.js canary vercel/next.js metadata-dynamic Change
_buildManifest.js gzip 484 B 482 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js metadata-dynamic Change
index.html gzip 527 B 529 B N/A
link.html gzip 540 B 542 B N/A
withRouter.html gzip 522 B 523 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js metadata-dynamic Change
edge-ssr.js gzip 96.1 kB 96.1 kB N/A
page.js gzip 140 kB 140 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js metadata-dynamic Change
middleware-b..fest.js gzip 625 B 623 B N/A
middleware-r..fest.js gzip 150 B 151 B N/A
middleware.js gzip 23 kB 23 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Diff details
Diff for page.js

Diff too large to display

Diff for 199-HASH.js

Diff too large to display

Commit: e9cc837

@kwonoj kwonoj changed the title [WIP] fix(metadata): align metadata suffix hash between turbopack fix(metadata): align metadata suffix hash between turbopack Oct 26, 2023
@kwonoj kwonoj marked this pull request as ready for review October 26, 2023 22:53
Comment on lines +276 to +277
result.reverse();
result[..6].iter().collect()
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()

@kodiakhq kodiakhq bot merged commit cce9f0d into canary Oct 30, 2023
51 of 57 checks passed
@kodiakhq kodiakhq bot deleted the metadata-dynamic branch October 30, 2023 19:56
@ForsakenHarmony
Copy link
Contributor

was actually looking into this and the rust version that more closely matches the js version would have been

fn djb2_hash(str: &str) -> u64 {
    let mut hash: f64 = 5381.0;
    for char in str.chars() {
        let code = char as u64 as f64;
        let tmp_hash = (hash as i64 as i32) << 5;
        hash = (tmp_hash as f64) + hash + code; 
    }
    hash.abs() as u64
}

but changing the js version is probably better

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

result.into_iter().rev().collect()
result.reverse();
result[..6].iter().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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Turbopack team PRs by the Turbopack team. locked Turbopack Related to Turbopack with Next.js. type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants