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

Add experimental trace file field #65071

Merged
merged 5 commits into from Apr 26, 2024
Merged

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Apr 26, 2024

This adds a new field to our .nft.json trace files when enabled behind a feature flag to output the hashes for the traced files.

Closes NEXT-3232

@ijjk
Copy link
Member Author

ijjk commented Apr 26, 2024

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary ijjk/next.js add/experimental-traces Change
buildDuration 18.8s 17.4s N/A
buildDurationCached 9.5s 8.2s N/A
nodeModulesSize 360 MB 360 MB ⚠️ +61.6 kB
nextStartRea..uration (ms) 457ms 473ms N/A
Client Bundles (main, webpack)
vercel/next.js canary ijjk/next.js add/experimental-traces Change
139-HASH.js gzip 31.8 kB 31.8 kB N/A
2478adb3-HASH.js gzip 53.5 kB 53.5 kB N/A
4967-HASH.js gzip 5.1 kB 5.1 kB N/A
6701.HASH.js gzip 168 B 168 B
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 229 B 227 B N/A
main-HASH.js gzip 31.6 kB 31.6 kB N/A
webpack-HASH.js gzip 1.65 kB 1.64 kB N/A
Overall change 45.3 kB 45.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary ijjk/next.js add/experimental-traces Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary ijjk/next.js add/experimental-traces Change
_app-HASH.js gzip 194 B 193 B N/A
_error-HASH.js gzip 192 B 192 B
amp-HASH.js gzip 510 B 510 B
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 2.51 kB 2.52 kB N/A
edge-ssr-HASH.js gzip 266 B 265 B N/A
head-HASH.js gzip 365 B 362 B N/A
hooks-HASH.js gzip 391 B 390 B N/A
image-HASH.js gzip 4.32 kB 4.32 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.69 kB 2.69 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 393 B 395 B N/A
withRouter-HASH.js gzip 323 B 323 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 1.73 kB 1.73 kB
Client Build Manifests
vercel/next.js canary ijjk/next.js add/experimental-traces Change
_buildManifest.js gzip 484 B 484 B
Overall change 484 B 484 B
Rendered Page Sizes
vercel/next.js canary ijjk/next.js add/experimental-traces Change
index.html gzip 527 B 528 B N/A
link.html gzip 540 B 541 B N/A
withRouter.html gzip 523 B 523 B
Overall change 523 B 523 B
Edge SSR bundle Size
vercel/next.js canary ijjk/next.js add/experimental-traces Change
edge-ssr.js gzip 108 kB 108 kB N/A
page.js gzip 3.04 kB 3.06 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary ijjk/next.js add/experimental-traces Change
middleware-b..fest.js gzip 622 B 624 B N/A
middleware-r..fest.js gzip 155 B 154 B N/A
middleware.js gzip 27.9 kB 27.9 kB
edge-runtime..pack.js gzip 839 B 839 B
Overall change 28.8 kB 28.8 kB
Next Runtimes
vercel/next.js canary ijjk/next.js add/experimental-traces Change
app-page-exp...dev.js gzip 171 kB 171 kB
app-page-exp..prod.js gzip 98.4 kB 98.4 kB
app-page-tur..prod.js gzip 99.9 kB 99.9 kB
app-page-tur..prod.js gzip 94.2 kB 94.2 kB
app-page.run...dev.js gzip 157 kB 157 kB
app-page.run..prod.js gzip 93 kB 93 kB
app-route-ex...dev.js gzip 21.4 kB 21.4 kB
app-route-ex..prod.js gzip 15.2 kB 15.2 kB
app-route-tu..prod.js gzip 15.2 kB 15.2 kB
app-route-tu..prod.js gzip 14.9 kB 14.9 kB
app-route.ru...dev.js gzip 21.3 kB 21.3 kB
app-route.ru..prod.js gzip 14.9 kB 14.9 kB
pages-api-tu..prod.js gzip 9.55 kB 9.55 kB
pages-api.ru...dev.js gzip 9.82 kB 9.82 kB
pages-api.ru..prod.js gzip 9.55 kB 9.55 kB
pages-turbo...prod.js gzip 21.4 kB 21.4 kB
pages.runtim...dev.js gzip 22.1 kB 22.1 kB
pages.runtim..prod.js gzip 21.4 kB 21.4 kB
server.runti..prod.js gzip 65.3 kB 65.3 kB
Overall change 975 kB 975 kB
build cache
vercel/next.js canary ijjk/next.js add/experimental-traces Change
0.pack gzip 1.61 MB 1.61 MB N/A
index.pack gzip 113 kB 112 kB N/A
Overall change 0 B 0 B
Diff details
Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Commit: ea31cfd

@ijjk ijjk marked this pull request as ready for review April 26, 2024 06:40
if (err) {
return reject(err)
}
resolve(result || '')
Copy link
Member

Choose a reason for hiding this comment

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

should we treat a missing result as an error too or is it ok for this to potentially return an empty string hash?

Copy link
Member Author

Choose a reason for hiding this comment

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

I took this as an empty file which is technically valid so don't think we should error

}
const fileHashes: Record<string, string> = {}

await Promise.all(
Copy link
Member

@ztanner ztanner Apr 26, 2024

Choose a reason for hiding this comment

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

should this be skipped ifflyingShuttle is disabled or is there still a use for fileHashes outside of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do skip the hashing part if flyingShuttle is disabled we still do the finalFiles collecting though

if (err) {
return reject(err)
}
resolve(getHash(result || ''))
Copy link
Member

Choose a reason for hiding this comment

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

same question about empty string on this one

Copy link
Member Author

Choose a reason for hiding this comment

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

should be the same as #65071 (comment)

if (this.flyingShuttle) {
this.traceHashes.set(
depMod.resource,
await getOriginalHash(depMod.resource)
Copy link
Member

Choose a reason for hiding this comment

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

should we Promise.all these dependency hashes?

Copy link
Member Author

@ijjk ijjk Apr 26, 2024

Choose a reason for hiding this comment

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

I didn't for this one as it's recursive and don't want to overload the stack/keep track of semaphore for it

@ijjk ijjk requested a review from ztanner April 26, 2024 17:06
@ijjk ijjk merged commit 4e84f69 into vercel:canary Apr 26, 2024
74 checks passed
@ijjk ijjk deleted the add/experimental-traces branch April 26, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants