-
Notifications
You must be signed in to change notification settings - Fork 523
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: migrate to node:fs/promises #8310
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. |
This pull request has merge conflicts that must be resolved before it can be merged. |
This pull request has merge conflicts that must be resolved before it can be merged. |
1 similar comment
This pull request has merge conflicts that must be resolved before it can be merged. |
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.
Sorry for the delay reviewing this, and the merge conflicts you've had to resolve as a result.
Generally, I'm not a fan of using an external lib where an inbuilt node one is sufficient. fs-extra
might be a little more elegant to check the existence of a file, but it enforces bad coding habits, i.e. checking if a file exists, then doing an operation on it, rather than doing an operation on it and catching the error if it doesn't exist.
Generally we want to do that, in some cases we check if files exist to display console messages or similar, in those cases we should use try/catch with fs.access
as I suggested in one of my comments.
import path from "node:path"; | ||
import zlib from "node:zlib"; | ||
|
||
import chalk from "chalk"; | ||
import cliProgress from "cli-progress"; | ||
import caporal from "@caporal/core"; | ||
import fse from "fs-extra"; |
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.
Not a fan of using an extra library here (I see we already use it in main
, we should remove it from there too) - the inbuilt node lib should be sufficient.
path.join(outPath, "metadata.json"), | ||
JSON.stringify(builtMetadata) | ||
); | ||
await fse.writeJson(path.join(outPath, "metadata.json"), builtMetadata); |
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.
Keep the prior JSON.stringify
, but use await fs.writeFile
- this applies to all introductions of fse.writeJson
.
if (await fse.pathExists(sitemapFilePath)) { | ||
sitemapsBuilt.push(sitemapFilePath); | ||
locales.push(locale); | ||
} |
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.
access
should work here instead, and all similar checks:
if (await fse.pathExists(sitemapFilePath)) { | |
sitemapsBuilt.push(sitemapFilePath); | |
locales.push(locale); | |
} | |
try { | |
await fs.access(sitemapFilePath); | |
sitemapsBuilt.push(sitemapFilePath); | |
locales.push(locale); | |
} catch { | |
if (e.code !== "ENOENT") throw e; | |
} |
const map = new Map(); | ||
if (previousFile) { | ||
const previous = JSON.parse(fs.readFileSync(previousFile, "utf-8")); | ||
const previous = await fse.readJson(previousFile, "utf-8"); |
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.
Keep JSON.parse
for this and all other additions of fse.ReadJson
:
const previous = await fse.readJson(previousFile, "utf-8"); | |
const previous = JSON.parse(await fs.readFile(previousFile, "utf-8")); |
if (await fse.pathExists(path.join(outPath, "index.html"))) { | ||
await fs.unlink(path.join(outPath, "index.html")); | ||
} |
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.
The node docs recommend against checking to see if a file exists, and then performing an op on it, otherwise a race condition can happen where the file is modified between the check and operation. Best to just do the operation, and catch it if it fails (do the same in the instance below):
if (await fse.pathExists(path.join(outPath, "index.html"))) { | |
await fs.unlink(path.join(outPath, "index.html")); | |
} | |
try { | |
await fs.unlink(path.join(outPath, "index.html")); | |
} catch (e) { | |
if (e.code !== "ENOENT") throw e; | |
} |
@@ -514,11 +515,11 @@ program | |||
let url: string; | |||
// Perhaps they typed in a path relative to the content root | |||
if ( | |||
(slug.startsWith("files") || fs.existsSync(slug)) && | |||
(slug.startsWith("files") || (await fse.pathExists(slug))) && |
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.
I think this whole command can be re-factored, here just attempt to load the split slug and if it fails move on.
slug.includes("translated-content") && | ||
!CONTENT_TRANSLATED_ROOT |
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.
This logic can exist in the catch
of the above file load, explaining possibly why it failed
(await fse.pathExists(slug)) && | ||
(await fse.pathExists(path.join(slug, "index.json"))) | ||
) { | ||
// Someone probably yarn `yarn build` and copy-n-pasted one of the lines | ||
// it spits out from its CLI. | ||
const { doc } = JSON.parse( | ||
fs.readFileSync(path.join(slug, "index.json"), "utf-8") | ||
const { doc } = await fse.readJson( | ||
path.join(slug, "index.json"), | ||
"utf-8" |
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.
Attempt to load the index.json
and catch if it fails
roots, | ||
loadHistory && fs.existsSync(loadHistory) ? loadHistory : null | ||
loadHistory && (await fse.pathExists(loadHistory)) ? loadHistory : null |
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.
Just delete loadHistory
, or set to null, above if fs.access(loadHistory)
fails
if (!refresh && (await fse.pathExists(outfile))) { | ||
const stat = await fs.stat(outfile); |
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.
Just catch the stat error
This PR converts various files and functions to use async (promises), specifically ones involving file access.