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

chore: reduce fs-extra usage in scripts/ #56917

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Oct 17, 2023

The PR follows #56536 and #56491, replacing fs-extra usages inside the scripts/ folder.

Note that the copy and move haven't been replaced yet. Currently, there is no better recursive copy (lightweight, promise-based, Node.js built-in copyFile API-based, support the filter option) library alternative available on npm, and Node.js built-in fs.rename doesn't support overwrite.

The PR also replaces many async fs API usage with their sync versions.

cc @wbinnssmith

@SukkaW SukkaW requested a review from a team as a code owner October 17, 2023 04:53
@ijjk
Copy link
Member

ijjk commented Oct 17, 2023

Tests Passed

@ijjk
Copy link
Member

ijjk commented Oct 17, 2023

Stats from current PR

Default Build
General
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
buildDuration 10.8s 10.7s N/A
buildDurationCached 6.3s 6.3s N/A
nodeModulesSize 174 MB 174 MB
nextStartRea..uration (ms) 516ms 517ms N/A
Client Bundles (main, webpack)
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
199-HASH.js gzip 27.5 kB 27.5 kB
3f784ff6-HASH.js gzip 53.1 kB 53.1 kB
99.HASH.js gzip 182 B 182 B
framework-HASH.js gzip 45.3 kB 45.3 kB
main-app-HASH.js gzip 254 B 252 B N/A
main-HASH.js gzip 32.9 kB 32.9 kB N/A
webpack-HASH.js gzip 1.75 kB 1.75 kB N/A
Overall change 126 kB 126 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary SukkaW/next.js scripts-fs-extra 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.57 kB 2.57 kB N/A
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.35 kB 4.35 kB N/A
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.64 kB 2.63 kB N/A
routerDirect..HASH.js gzip 312 B 311 B N/A
script-HASH.js gzip 385 B 384 B N/A
withRouter-HASH.js gzip 307 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 1.08 kB 1.08 kB
Client Build Manifests
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
_buildManifest.js gzip 485 B 482 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
index.html gzip 528 B 530 B N/A
link.html gzip 541 B 542 B N/A
withRouter.html gzip 523 B 524 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
edge-ssr.js gzip 93.6 kB 93.6 kB N/A
page.js gzip 154 kB 154 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary SukkaW/next.js scripts-fs-extra Change
middleware-b..fest.js gzip 626 B 621 B N/A
middleware-r..fest.js gzip 150 B 151 B N/A
middleware.js gzip 22.5 kB 22.5 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB
Overall change 1.92 kB 1.92 kB
Commit: 04dde74

@@ -1,7 +1,8 @@
import os from 'os'
import path from 'path'
import execa from 'execa'
import fs from 'fs-extra'
import fs from 'fs'
Copy link
Member

Choose a reason for hiding this comment

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

Can we use promises here instead of sync funcs?

Copy link
Contributor Author

@SukkaW SukkaW Oct 17, 2023

Choose a reason for hiding this comment

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

Even with promises-based fs APIs, the main thread is still blocked by await in the single-threaded script, effectively idling (install-native.mjs is a single-thread script, invoked by pnpm run).

fs async APIs are favored when the main thread should not be blocked, such as a web server managing several incoming requests, or using Promise.all to parallelize tasks sent in the Node.js's libuv worker pool. But we are not doing those in install-native.mjs.

Generally, fs sync APIs are faster (up to 40%) than async APIs (They don't require creating Promise objects, which alleviates GC pressure and avoids extra ticks).

scripts/publish-release.js Outdated Show resolved Hide resolved
@SukkaW SukkaW requested a review from styfle October 17, 2023 16:50
@kodiakhq kodiakhq bot merged commit 17553c5 into vercel:canary Oct 17, 2023
52 of 57 checks passed
styfle added a commit that referenced this pull request Oct 17, 2023
kodiakhq bot pushed a commit that referenced this pull request Oct 19, 2023
Follows #56917, revert back to moving files using `fs-extra`. This should help with the flaky `fs.rename` operation on Windows.

cc @styfle
@SukkaW SukkaW deleted the scripts-fs-extra branch October 24, 2023 08:33
@github-actions github-actions bot added the locked label Nov 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants