chore: migrate to node:fs/promises #8310

queengooborg · 2023-02-25T07:03:24Z

This PR converts various files and functions to use async (promises), specifically ones involving file access.

github-actions · 2023-02-28T09:33:36Z

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions · 2023-03-06T15:09:09Z

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions · 2023-05-03T15:54:54Z

This pull request has merge conflicts that must be resolved before it can be merged.

github-actions · 2023-05-03T15:54:54Z

This pull request has merge conflicts that must be resolved before it can be merged.

LeoMcA

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.

LeoMcA · 2023-05-18T10:50:10Z

build/cli.ts

 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";


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.

LeoMcA · 2023-05-18T10:53:33Z

build/cli.ts

-      path.join(outPath, "metadata.json"),
-      JSON.stringify(builtMetadata)
-    );
+    await fse.writeJson(path.join(outPath, "metadata.json"), builtMetadata);


Keep the prior JSON.stringify, but use await fs.writeFile - this applies to all introductions of fse.writeJson.

LeoMcA · 2023-05-18T10:58:30Z

build/cli.ts

+          if (await fse.pathExists(sitemapFilePath)) {
            sitemapsBuilt.push(sitemapFilePath);
            locales.push(locale);
          }


access should work here instead, and all similar checks:

Suggested change

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;

}

LeoMcA · 2023-05-18T10:59:45Z

build/git-history.ts

  const map = new Map();
  if (previousFile) {
-    const previous = JSON.parse(fs.readFileSync(previousFile, "utf-8"));
+    const previous = await fse.readJson(previousFile, "utf-8");


Keep JSON.parse for this and all other additions of fse.ReadJson:

Suggested change

const previous = await fse.readJson(previousFile, "utf-8");

const previous = JSON.parse(await fs.readFile(previousFile, "utf-8"));

LeoMcA · 2023-05-18T11:17:26Z

server/document.ts

+    if (await fse.pathExists(path.join(outPath, "index.html"))) {
+      await fs.unlink(path.join(outPath, "index.html"));
    }


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):

Suggested change

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;

}

LeoMcA · 2023-05-18T11:57:05Z

tool/cli.ts

@@ -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))) &&


I think this whole command can be re-factored, here just attempt to load the split slug and if it fails move on.

LeoMcA · 2023-05-18T11:57:30Z

tool/cli.ts

          slug.includes("translated-content") &&
          !CONTENT_TRANSLATED_ROOT


This logic can exist in the catch of the above file load, explaining possibly why it failed

LeoMcA · 2023-05-18T11:58:00Z

tool/cli.ts

+        (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"


Attempt to load the index.json and catch if it fails

LeoMcA · 2023-05-18T12:01:30Z

tool/cli.ts

        roots,
-        loadHistory && fs.existsSync(loadHistory) ? loadHistory : null
+        loadHistory && (await fse.pathExists(loadHistory)) ? loadHistory : null


Just delete loadHistory, or set to null, above if fs.access(loadHistory) fails

LeoMcA · 2023-05-18T12:02:14Z

tool/cli.ts

+      if (!refresh && (await fse.pathExists(outfile))) {
+        const stat = await fs.stat(outfile);


Just catch the stat error

queengooborg added 7 commits February 24, 2023 21:08

Convert testing/tests/* to async

d59abd2

Convert kumascript/tests/* to async

407abc3

Convert server/* to async

ddd0184

Convert tool/* to async

4270003

Convert ssr/* to async

28d0d73

Convert build/* to async

52849b0

Convert testing/tests/destructive.test.ts to async

716a733

github-actions bot added the macros tracking issues related to kumascript macros label Feb 25, 2023

Fix missing await declarations

2457e35

queengooborg marked this pull request as draft February 25, 2023 07:24

Add missing async/await declarations

5069d6d

queengooborg marked this pull request as ready for review February 25, 2023 08:51

queengooborg changed the title ~~perf: Convert various functions to async~~ refactor: Convert various functions to async Feb 25, 2023

github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 28, 2023

Merge branch 'main' into fs-promises/main

4d9afa3

github-actions bot added merge conflicts 🚧 Please rebase onto or merge the latest main. and removed merge conflicts 🚧 Please rebase onto or merge the latest main. labels Feb 28, 2023

Merge branch 'main' into fs-promises/main

58bc739

github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Mar 16, 2023

github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label May 3, 2023

LeoMcA requested changes May 18, 2023

View reviewed changes

LeoMcA changed the title ~~refactor: Convert various functions to async~~ chore: migrate to node:fs/promises May 18, 2023

queengooborg closed this Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate to node:fs/promises #8310

chore: migrate to node:fs/promises #8310

queengooborg commented Feb 25, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023

github-actions bot commented Mar 6, 2023

github-actions bot commented May 3, 2023

github-actions bot commented May 3, 2023

LeoMcA left a comment

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

LeoMcA May 18, 2023

	const previous = await fse.readJson(previousFile, "utf-8");
	const previous = JSON.parse(await fs.readFile(previousFile, "utf-8"));

		slug.includes("translated-content") &&
		!CONTENT_TRANSLATED_ROOT

		if (!refresh && (await fse.pathExists(outfile))) {
		const stat = await fs.stat(outfile);

chore: migrate to node:fs/promises #8310

chore: migrate to node:fs/promises #8310

Conversation

queengooborg commented Feb 25, 2023 • edited Loading

github-actions bot commented Feb 28, 2023

github-actions bot commented Mar 6, 2023

github-actions bot commented May 3, 2023

github-actions bot commented May 3, 2023

LeoMcA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

queengooborg commented Feb 25, 2023 •

edited

Loading