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

Avoid fs write next-env.d.ts on read-only filesystems #28206

Merged
merged 4 commits into from Aug 18, 2021
Merged

Avoid fs write next-env.d.ts on read-only filesystems #28206

merged 4 commits into from Aug 18, 2021

Conversation

chrislloyd
Copy link
Contributor

@chrislloyd chrislloyd commented Aug 17, 2021

Next.js currently writes the TS type declarations on startup, regardless of the existing content of the file. This is good for ensuring the file content stays consistent. However, if the file content is already correct, this will perform an unnessecary write.

When running Next in read-only filesystems (such as the Bazel sandbox) this can cause the build to fail even if the content of the type declaration file is already correct.

This fixes this by only writing the contents of the file if the current contents don't match.

Test Plan

Added an integration test for the general behavior of writing next-env.d.ts.

@chrislloyd chrislloyd marked this pull request as ready for review August 17, 2021 17:18
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Thanks!

Let's also add a test for this new behavior

@styfle styfle changed the title Avoid a fs write for read-only filesystems Avoid fs write next-env.d.ts on read-only filesystems Aug 17, 2021
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! 🎉

@ijjk
Copy link
Member

ijjk commented Aug 18, 2021

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
buildDuration 12.3s 12.8s ⚠️ +470ms
buildDurationCached 3.2s 3.1s -154ms
nodeModulesSize 61.5 MB 61.5 MB ⚠️ +714 B
Page Load Tests Overall increase ✓
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
/ failed reqs 0 0
/ total time (seconds) 2.223 2.122 -0.1
/ avg req/sec 1124.69 1178.03 +53.34
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.262 1.237 -0.02
/error-in-render avg req/sec 1981.16 2020.97 +39.81
Client Bundles (main, webpack, commons)
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
745.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 23.2 kB 23.2 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 67.1 kB 67.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
polyfills-a4..dd70.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
_app-HASH.js gzip 979 B 979 B
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 329 B 329 B
dynamic-HASH.js gzip 2.65 kB 2.65 kB
head-HASH.js gzip 351 B 351 B
hooks-HASH.js gzip 918 B 918 B
image-HASH.js gzip 4.13 kB 4.13 kB
index-HASH.js gzip 261 B 261 B
link-HASH.js gzip 1.66 kB 1.66 kB
routerDirect..HASH.js gzip 318 B 318 B
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 320 B 320 B
bb14e60e810b..30f.css gzip 125 B 125 B
Overall change 12.9 kB 12.9 kB
Client Build Manifests
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
_buildManifest.js gzip 492 B 492 B
Overall change 492 B 492 B
Rendered Page Sizes
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
index.html gzip 539 B 539 B
link.html gzip 551 B 551 B
withRouter.html gzip 532 B 532 B
Overall change 1.62 kB 1.62 kB

Webpack 4 Mode (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
buildDuration 10.6s 10.5s -124ms
buildDurationCached 4.3s 4.3s ⚠️ +90ms
nodeModulesSize 61.5 MB 61.5 MB ⚠️ +714 B
Page Load Tests Overall increase ✓
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
/ failed reqs 0 0
/ total time (seconds) 2.418 2.331 -0.09
/ avg req/sec 1033.91 1072.44 +38.53
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.338 1.344 ⚠️ +0.01
/error-in-render avg req/sec 1868.29 1860.66 ⚠️ -7.63
Client Bundles (main, webpack, commons)
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
16.HASH.js gzip 186 B 186 B
677f882d2ed8..HASH.js gzip 14.1 kB 14.1 kB
framework.HASH.js gzip 41.9 kB 41.9 kB
main-HASH.js gzip 10.6 kB 10.6 kB
webpack-HASH.js gzip 1.19 kB 1.19 kB
Overall change 68 kB 68 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
polyfills-a4..dd70.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
_app-HASH.js gzip 964 B 964 B
_error-HASH.js gzip 3.8 kB 3.8 kB
amp-HASH.js gzip 552 B 552 B
css-HASH.js gzip 333 B 333 B
dynamic-HASH.js gzip 2.85 kB 2.85 kB
head-HASH.js gzip 3.06 kB 3.06 kB
hooks-HASH.js gzip 924 B 924 B
index-HASH.js gzip 231 B 231 B
link-HASH.js gzip 1.64 kB 1.64 kB
routerDirect..HASH.js gzip 298 B 298 B
script-HASH.js gzip 2.98 kB 2.98 kB
withRouter-HASH.js gzip 295 B 295 B
30809af5c834..565.css gzip 125 B 125 B
Overall change 18.1 kB 18.1 kB
Client Build Manifests
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
_buildManifest.js gzip 500 B 500 B
Overall change 500 B 500 B
Rendered Page Sizes
vercel/next.js canary chrislloyd/next.js fix-type-declarations-write Change
index.html gzip 585 B 585 B
link.html gzip 597 B 597 B
withRouter.html gzip 578 B 578 B
Overall change 1.76 kB 1.76 kB
Commit: c593ef0

Comment on lines +29 to +30
(await fileExists(appTypeDeclarations)) &&
(await fs.readFile(appTypeDeclarations, 'utf8')) === content
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(await fileExists(appTypeDeclarations)) &&
(await fs.readFile(appTypeDeclarations, 'utf8')) === content
(await fs.readFile(appTypeDeclarations, 'utf8').catch(() => '') === content

Instead of checking if the file exists and then reading the content we could read the content and handle it failing gracefully which seems to be recommended in the node docs here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. That's a good point and would have fixed it if the merge queue hadn't kicked in. I think practically, that case is unlikely to be an issue here - the window where the user would have to write the file is slim and the consequence is small. Is it worth a PR to fix?

Copy link
Member

@ijjk ijjk Aug 18, 2021

Choose a reason for hiding this comment

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

Yeah makes sense, I think it's alright to leave as is.

@kodiakhq kodiakhq bot merged commit 52c2f8b into vercel:canary Aug 18, 2021
@chrislloyd chrislloyd deleted the fix-type-declarations-write branch August 18, 2021 04:11
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants