-
Notifications
You must be signed in to change notification settings - Fork 27k
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
perf: download and save mkcert in stream #63527
Conversation
Failing test suitesCommit: e0e27cf
Expand output● app-dir - params hooks compat › should only access search params with useSearchParams
Read more about building and testing Next.js in contributing.md. |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
buildDuration | 17.1s | 17.3s | |
buildDurationCached | 10.7s | 8s | N/A |
nodeModulesSize | 198 MB | 198 MB | |
nextStartRea..uration (ms) | 456ms | 468ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
2453-HASH.js gzip | 31 kB | 31 kB | N/A |
3304.HASH.js gzip | 181 B | 181 B | ✓ |
3f784ff6-HASH.js gzip | 53.7 kB | 53.7 kB | ✓ |
8299-HASH.js gzip | 5.04 kB | 5.04 kB | N/A |
framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
main-app-HASH.js gzip | 242 B | 242 B | ✓ |
main-HASH.js gzip | 32.2 kB | 32.2 kB | N/A |
webpack-HASH.js gzip | 1.68 kB | 1.68 kB | N/A |
Overall change | 99.3 kB | 99.3 kB | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
_app-HASH.js gzip | 196 B | 197 B | N/A |
_error-HASH.js gzip | 184 B | 184 B | ✓ |
amp-HASH.js gzip | 505 B | 505 B | ✓ |
css-HASH.js gzip | 324 B | 325 B | N/A |
dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | N/A |
edge-ssr-HASH.js gzip | 258 B | 258 B | ✓ |
head-HASH.js gzip | 352 B | 352 B | ✓ |
hooks-HASH.js gzip | 370 B | 371 B | N/A |
image-HASH.js gzip | 4.21 kB | 4.21 kB | ✓ |
index-HASH.js gzip | 259 B | 259 B | ✓ |
link-HASH.js gzip | 2.67 kB | 2.67 kB | N/A |
routerDirect..HASH.js gzip | 314 B | 312 B | N/A |
script-HASH.js gzip | 386 B | 386 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 6.57 kB | 6.57 kB | ✓ |
Client Build Manifests
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
_buildManifest.js gzip | 481 B | 484 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
index.html gzip | 529 B | 529 B | ✓ |
link.html gzip | 541 B | 541 B | ✓ |
withRouter.html gzip | 524 B | 523 B | N/A |
Overall change | 1.07 kB | 1.07 kB | ✓ |
Edge SSR bundle Size
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
edge-ssr.js gzip | 95.3 kB | 95.3 kB | N/A |
page.js gzip | 3.04 kB | 3.04 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 625 B | 626 B | N/A |
middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
middleware.js gzip | 25.5 kB | 25.5 kB | N/A |
edge-runtime..pack.js gzip | 839 B | 839 B | ✓ |
Overall change | 990 B | 990 B | ✓ |
Next Runtimes
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 170 kB | 170 kB | ✓ |
app-page-exp..prod.js gzip | 97 kB | 97 kB | ✓ |
app-page-tur..prod.js gzip | 98.7 kB | 98.7 kB | ✓ |
app-page-tur..prod.js gzip | 93 kB | 93 kB | ✓ |
app-page.run...dev.js gzip | 144 kB | 144 kB | ✓ |
app-page.run..prod.js gzip | 91.5 kB | 91.5 kB | ✓ |
app-route-ex...dev.js gzip | 21.4 kB | 21.4 kB | ✓ |
app-route-ex..prod.js gzip | 15.1 kB | 15.1 kB | ✓ |
app-route-tu..prod.js gzip | 15.1 kB | 15.1 kB | ✓ |
app-route-tu..prod.js gzip | 14.8 kB | 14.8 kB | ✓ |
app-route.ru...dev.js gzip | 21 kB | 21 kB | ✓ |
app-route.ru..prod.js gzip | 14.8 kB | 14.8 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 | 22.5 kB | 22.5 kB | ✓ |
pages.runtim...dev.js gzip | 23.1 kB | 23.1 kB | ✓ |
pages.runtim..prod.js gzip | 22.4 kB | 22.4 kB | ✓ |
server.runti..prod.js gzip | 51 kB | 51 kB | ✓ |
Overall change | 944 kB | 944 kB | ✓ |
build cache Overall increase ⚠️
vercel/next.js canary | SukkaW/next.js stream-download-mkcert | Change | |
---|---|---|---|
0.pack gzip | 1.57 MB | 1.57 MB | |
index.pack gzip | 106 kB | 105 kB | N/A |
Overall change | 1.57 MB | 1.57 MB |
Diff details
Diff for middleware.js
Diff too large to display
packages/next/src/lib/mkcert.ts
Outdated
const arrayBuffer = await response.arrayBuffer() | ||
const buffer = Buffer.from(arrayBuffer) | ||
await pipeline( | ||
Readable.fromWeb(response.body as any), // type definition of ReadableStream from @types/node and lib.dom.d.ts are incompatible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we avoid .fromWeb
as it's still marked as experimental stage 1?
x-ref: https://nodejs.org/api/stream.html#streamreadablefromwebreadablestream-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should not use an experimental api.
All things considered, we should ditch the fetch
here and instead use the significantly faster undici.pipeline
or undici.stream
. Keep it all in Node.js streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is out of scope of this pr, I recommend manually reading the web stream returned by fetch and writing it to the file in chunks.
You may also see if one of Node.js' write operations supports an async iterable and then you can probably pass the response body directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ijjk @Ethan-Arrowood Next.js has removed undici
from its dependency, which means we no longer have access to undici.pipeline
or undici.stream
without re-introducing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to use DetachedPromise
here. But I won't block on it.
Previously, Next.js would buffer the
mkcert
binary in memory and write it to disk all at once. The pull request changes this to pipe the download stream to the disk. This improves memory consumption by avoiding having to load the entiremkcert
binary into memory.