fix: Node polyfill fsAppend rework (#4322)

* My original implementation of `fs.appendFile` used an async API, which, though 
  it would work fine as a polyfill, wasn't an exact match with the Node API.  This PR
  reworks that API to mimic the Node API fully as a synchronous void function with
  an async internal implementation.
* Refactor move of other internal fs `dirent` and `dir` classes to the _fs internal
  directory.

7 files changed, 113 insertions, 89 deletions

diff --git a/std/node/_fs/_fs_appendFile.ts b/std/node/_fs/_fs_appendFile.ts
index 962f44884..193badf1f 100644
--- a/std/node/_fs/_fs_appendFile.ts
+++ b/std/node/_fs/_fs_appendFile.ts
@@ -1,18 +1,18 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
-import { FileOptions, isFileOptions } from "./_fs_common.ts";
+import { FileOptions, isFileOptions, CallbackWithError } from "./_fs_common.ts";
 import { notImplemented } from "../_utils.ts";
 
 /**
  * TODO: Also accept 'data' parameter as a Node polyfill Buffer type once this
  * is implemented. See https://github.com/denoland/deno/issues/3403
  */
-export async function appendFile(
+export function appendFile(
   pathOrRid: string | number,
   data: string,
-  optionsOrCallback: string | FileOptions | Function,
-  callback?: Function
-): Promise<void> {
-  const callbackFn: Function | undefined =
+  optionsOrCallback: string | FileOptions | CallbackWithError,
+  callback?: CallbackWithError
+): void {
+  const callbackFn: CallbackWithError | undefined =
     optionsOrCallback instanceof Function ? optionsOrCallback : callback;
   const options: string | FileOptions | undefined =
     optionsOrCallback instanceof Function ? undefined : optionsOrCallback;
@@ -23,37 +23,48 @@ export async function appendFile(
   validateEncoding(options);
 
   let rid = -1;
-  try {
-    if (typeof pathOrRid === "number") {
-      rid = pathOrRid;
-    } else {
-      const mode: number | undefined = isFileOptions(options)
-        ? options.mode
-        : undefined;
-      const flag: string | undefined = isFileOptions(options)
-        ? options.flag
-        : undefined;
-
-      if (mode) {
-        //TODO rework once https://github.com/denoland/deno/issues/4017 completes
-        notImplemented("Deno does not yet support setting mode on create");
+  new Promise(async (resolve, reject) => {
+    try {
+      if (typeof pathOrRid === "number") {
+        rid = pathOrRid;
+      } else {
+        const mode: number | undefined = isFileOptions(options)
+          ? options.mode
+          : undefined;
+        const flag: string | undefined = isFileOptions(options)
+          ? options.flag
+          : undefined;
+
+        if (mode) {
+          //TODO rework once https://github.com/denoland/deno/issues/4017 completes
+          notImplemented("Deno does not yet support setting mode on create");
+        }
+        const file = await Deno.open(pathOrRid, getOpenOptions(flag));
+        rid = file.rid;
       }
 
-      const file = await Deno.open(pathOrRid, getOpenOptions(flag));
-      rid = file.rid;
-    }
-
-    const buffer: Uint8Array = new TextEncoder().encode(data);
+      const buffer: Uint8Array = new TextEncoder().encode(data);
+
+      await Deno.write(rid, buffer);
+      resolve();
+    } catch (err) {
+      reject(err);
+    }
+  })
+    .then(() => {
+      closeRidIfNecessary(typeof pathOrRid === "string", rid);
+      callbackFn();
+    })
+    .catch(err => {
+      closeRidIfNecessary(typeof pathOrRid === "string", rid);
+      callbackFn(err);
+    });
+}
 
-    await Deno.write(rid, buffer);
-    callbackFn();
-  } catch (err) {
-    callbackFn(err);
-  } finally {
-    if (typeof pathOrRid === "string" && rid != -1) {
-      //Only close if a path was supplied and a rid allocated
-      Deno.close(rid);
-    }
+function closeRidIfNecessary(isPathString: boolean, rid: number): void {
+  if (isPathString && rid != -1) {
+    //Only close if a path was supplied and a rid allocated
+    Deno.close(rid);
   }
 }
 
@@ -94,10 +105,7 @@ export function appendFileSync(
 
     Deno.writeSync(rid, buffer);
   } finally {
-    if (typeof pathOrRid === "string" && rid != -1) {
-      //Only close if a 'string' path was supplied and a rid allocated
-      Deno.close(rid);
-    }
+    closeRidIfNecessary(typeof pathOrRid === "string", rid);
   }
 }
 
diff --git a/std/node/_fs/_fs_appendFile_test.ts b/std/node/_fs/_fs_appendFile_test.ts
index dcea69783..47d552740 100644
--- a/std/node/_fs/_fs_appendFile_test.ts
+++ b/std/node/_fs/_fs_appendFile_test.ts
@@ -1,21 +1,16 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
 const { test } = Deno;
-import {
-  assertEquals,
-  assert,
-  assertThrows,
-  assertThrowsAsync
-} from "../../testing/asserts.ts";
+import { assertEquals, assertThrows, fail } from "../../testing/asserts.ts";
 import { appendFile, appendFileSync } from "./_fs_appendFile.ts";
 
 const decoder = new TextDecoder("utf-8");
 
 test({
   name: "No callback Fn results in Error",
-  async fn() {
-    await assertThrowsAsync(
-      async () => {
-        await appendFile("some/path", "some data", "utf8");
+  fn() {
+    assertThrows(
+      () => {
+        appendFile("some/path", "some data", "utf8");
       },
       Error,
       "No callback function supplied"
@@ -25,22 +20,17 @@ test({
 
 test({
   name: "Unsupported encoding results in error()",
-  async fn() {
-    await assertThrowsAsync(
-      async () => {
-        await appendFile(
-          "some/path",
-          "some data",
-          "made-up-encoding",
-          () => {}
-        );
+  fn() {
+    assertThrows(
+      () => {
+        appendFile("some/path", "some data", "made-up-encoding", () => {});
       },
       Error,
       "Only 'utf8' encoding is currently supported"
     );
-    await assertThrowsAsync(
-      async () => {
-        await appendFile(
+    assertThrows(
+      () => {
+        appendFile(
           "some/path",
           "some data",
           { encoding: "made-up-encoding" },
@@ -75,31 +65,47 @@ test({
       write: true,
       read: true
     });
-    let calledBack = false;
-    await appendFile(file.rid, "hello world", () => {
-      calledBack = true;
-    });
-    assert(calledBack);
-    Deno.close(file.rid);
-    const data = await Deno.readFile(tempFile);
-    assertEquals(decoder.decode(data), "hello world");
-    await Deno.remove(tempFile);
+    await new Promise((resolve, reject) => {
+      appendFile(file.rid, "hello world", err => {
+        if (err) reject();
+        else resolve();
+      });
+    })
+      .then(async () => {
+        const data = await Deno.readFile(tempFile);
+        assertEquals(decoder.decode(data), "hello world");
+      })
+      .catch(() => {
+        fail("No error expected");
+      })
+      .finally(async () => {
+        Deno.close(file.rid);
+        await Deno.remove(tempFile);
+      });
   }
 });
 
 test({
   name: "Async: Data is written to passed in file path",
   async fn() {
-    let calledBack = false;
     const openResourcesBeforeAppend: Deno.ResourceMap = Deno.resources();
-    await appendFile("_fs_appendFile_test_file.txt", "hello world", () => {
-      calledBack = true;
-    });
-    assert(calledBack);
-    assertEquals(Deno.resources(), openResourcesBeforeAppend);
-    const data = await Deno.readFile("_fs_appendFile_test_file.txt");
-    assertEquals(decoder.decode(data), "hello world");
-    await Deno.remove("_fs_appendFile_test_file.txt");
+    await new Promise((resolve, reject) => {
+      appendFile("_fs_appendFile_test_file.txt", "hello world", err => {
+        if (err) reject(err);
+        else resolve();
+      });
+    })
+      .then(async () => {
+        assertEquals(Deno.resources(), openResourcesBeforeAppend);
+        const data = await Deno.readFile("_fs_appendFile_test_file.txt");
+        assertEquals(decoder.decode(data), "hello world");
+      })
+      .catch(err => {
+        fail("No error was expected: " + err);
+      })
+      .finally(async () => {
+        await Deno.remove("_fs_appendFile_test_file.txt");
+      });
   }
 });
 
@@ -107,16 +113,23 @@ test({
   name:
     "Async: Callback is made with error if attempting to append data to an existing file with 'ax' flag",
   async fn() {
-    let calledBack = false;
     const openResourcesBeforeAppend: Deno.ResourceMap = Deno.resources();
     const tempFile: string = await Deno.makeTempFile();
-    await appendFile(tempFile, "hello world", { flag: "ax" }, (err: Error) => {
-      calledBack = true;
-      assert(err);
-    });
-    assert(calledBack);
-    assertEquals(Deno.resources(), openResourcesBeforeAppend);
-    await Deno.remove(tempFile);
+    await new Promise((resolve, reject) => {
+      appendFile(tempFile, "hello world", { flag: "ax" }, err => {
+        if (err) reject(err);
+        else resolve();
+      });
+    })
+      .then(() => {
+        fail("Expected error to be thrown");
+      })
+      .catch(() => {
+        assertEquals(Deno.resources(), openResourcesBeforeAppend);
+      })
+      .finally(async () => {
+        await Deno.remove(tempFile);
+      });
   }
 });
 
diff --git a/std/node/_fs/_fs_common.ts b/std/node/_fs/_fs_common.ts
index 4de0899ea..ebdd28d64 100644
--- a/std/node/_fs/_fs_common.ts
+++ b/std/node/_fs/_fs_common.ts
@@ -1,4 +1,7 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
+
+export type CallbackWithError = (err?: Error) => void;
+
 export interface FileOptions {
   encoding?: string;
   mode?: number;
diff --git a/std/node/_fs_dir.ts b/std/node/_fs/_fs_dir.ts
index e3830bb31..e3830bb31 100644
--- a/std/node/_fs_dir.ts
+++ b/std/node/_fs/_fs_dir.ts
diff --git a/std/node/_fs_dir_test.ts b/std/node/_fs/_fs_dir_test.ts
index f2ee814c2..aefa85854 100644
--- a/std/node/_fs_dir_test.ts
+++ b/std/node/_fs/_fs_dir_test.ts
@@ -1,5 +1,5 @@
 const { test } = Deno;
-import { assert, assertEquals, fail } from "../testing/asserts.ts";
+import { assert, assertEquals, fail } from "../../testing/asserts.ts";
 import Dir from "./_fs_dir.ts";
 import Dirent from "./_fs_dirent.ts";
 
diff --git a/std/node/_fs_dirent.ts b/std/node/_fs/_fs_dirent.ts
index 74447f404..38cd23d88 100644
--- a/std/node/_fs_dirent.ts
+++ b/std/node/_fs/_fs_dirent.ts
@@ -1,4 +1,4 @@
-import { notImplemented } from "./_utils.ts";
+import { notImplemented } from "../_utils.ts";
 
 export default class Dirent {
   constructor(private entry: Deno.FileInfo) {}
diff --git a/std/node/_fs_dirent_test.ts b/std/node/_fs/_fs_dirent_test.ts
index acbaffc50..6fe0f4020 100644
--- a/std/node/_fs_dirent_test.ts
+++ b/std/node/_fs/_fs_dirent_test.ts
@@ -1,5 +1,5 @@
 const { test } = Deno;
-import { assert, assertEquals, assertThrows } from "../testing/asserts.ts";
+import { assert, assertEquals, assertThrows } from "../../testing/asserts.ts";
 import Dirent from "./_fs_dirent.ts";
 
 class FileInfoMock implements Deno.FileInfo {