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, 544 insertions, 86 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/_fs_dir.ts b/std/node/_fs/_fs_dir.ts
new file mode 100644
index 000000000..e3830bb31
--- /dev/null
+++ b/std/node/_fs/_fs_dir.ts
@@ -0,0 +1,114 @@
+import Dirent from "./_fs_dirent.ts";
+
+export default class Dir {
+  private dirPath: string | Uint8Array;
+  private files: Dirent[] = [];
+  private filesReadComplete = false;
+
+  constructor(path: string | Uint8Array) {
+    this.dirPath = path;
+  }
+
+  get path(): string {
+    if (this.dirPath instanceof Uint8Array) {
+      return new TextDecoder().decode(this.dirPath);
+    }
+    return this.dirPath;
+  }
+
+  /**
+   * NOTE: Deno doesn't provide an interface to the filesystem like readdir
+   * where each call to readdir returns the next file.  This function simulates this
+   * behaviour by fetching all the entries on the first call, putting them on a stack
+   * and then popping them off the stack one at a time.
+   *
+   * TODO: Rework this implementation once https://github.com/denoland/deno/issues/4218
+   * is resolved.
+   */
+  read(callback?: Function): Promise<Dirent | null> {
+    return new Promise(async (resolve, reject) => {
+      try {
+        if (this.initializationOfDirectoryFilesIsRequired()) {
+          const denoFiles: Deno.FileInfo[] = await Deno.readdir(this.path);
+          this.files = denoFiles.map(file => new Dirent(file));
+        }
+        const nextFile = this.files.pop();
+        if (nextFile) {
+          resolve(nextFile);
+          this.filesReadComplete = this.files.length === 0;
+        } else {
+          this.filesReadComplete = true;
+          resolve(null);
+        }
+        if (callback) {
+          callback(null, !nextFile ? null : nextFile);
+        }
+      } catch (err) {
+        if (callback) {
+          callback(err, null);
+        }
+        reject(err);
+      }
+    });
+  }
+
+  readSync(): Dirent | null {
+    if (this.initializationOfDirectoryFilesIsRequired()) {
+      this.files.push(
+        ...Deno.readdirSync(this.path).map(file => new Dirent(file))
+      );
+    }
+    const dirent: Dirent | undefined = this.files.pop();
+    this.filesReadComplete = this.files.length === 0;
+
+    return !dirent ? null : dirent;
+  }
+
+  private initializationOfDirectoryFilesIsRequired(): boolean {
+    return this.files.length === 0 && !this.filesReadComplete;
+  }
+
+  /**
+   * Unlike Node, Deno does not require managing resource ids for reading
+   * directories, and therefore does not need to close directories when
+   * finished reading.
+   */
+  close(callback?: Function): Promise<void> {
+    return new Promise((resolve, reject) => {
+      try {
+        if (callback) {
+          callback(null);
+        }
+        resolve();
+      } catch (err) {
+        if (callback) {
+          callback(err);
+        }
+        reject(err);
+      }
+    });
+  }
+
+  /**
+   * Unlike Node, Deno does not require managing resource ids for reading
+   * directories, and therefore does not need to close directories when
+   * finished reading
+   */
+  closeSync(): void {
+    //No op
+  }
+
+  async *[Symbol.asyncIterator](): AsyncIterableIterator<Dirent> {
+    try {
+      while (true) {
+        const dirent: Dirent | null = await this.read();
+        if (dirent === null) {
+          break;
+        }
+        yield dirent;
+      }
+    } finally {
+      await this.close();
+    }
+  }
+}
diff --git a/std/node/_fs/_fs_dir_test.ts b/std/node/_fs/_fs_dir_test.ts
new file mode 100644
index 000000000..aefa85854
--- /dev/null
+++ b/std/node/_fs/_fs_dir_test.ts
@@ -0,0 +1,156 @@
+const { test } = Deno;
+import { assert, assertEquals, fail } from "../../testing/asserts.ts";
+import Dir from "./_fs_dir.ts";
+import Dirent from "./_fs_dirent.ts";
+
+test({
+  name: "Closing current directory with callback is successful",
+  async fn() {
+    let calledBack = false;
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    new Dir(".").close((valOrErr: any) => {
+      assert(!valOrErr);
+      calledBack = true;
+    });
+    assert(calledBack);
+  }
+});
+
+test({
+  name: "Closing current directory without callback returns void Promise",
+  async fn() {
+    await new Dir(".").close();
+  }
+});
+
+test({
+  name: "Closing current directory synchronously works",
+  async fn() {
+    new Dir(".").closeSync();
+  }
+});
+
+test({
+  name: "Path is correctly returned",
+  fn() {
+    assertEquals(new Dir("std/node").path, "std/node");
+
+    const enc: Uint8Array = new TextEncoder().encode("std/node");
+    assertEquals(new Dir(enc).path, "std/node");
+  }
+});
+
+test({
+  name: "read returns null for empty directory",
+  async fn() {
+    const testDir: string = Deno.makeTempDirSync();
+    try {
+      const file: Dirent | null = await new Dir(testDir).read();
+      assert(file === null);
+
+      let calledBack = false;
+      const fileFromCallback: Dirent | null = await new Dir(
+        testDir
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+      ).read((err: any, res: Dirent) => {
+        assert(res === null);
+        assert(err === null);
+        calledBack = true;
+      });
+      assert(fileFromCallback === null);
+      assert(calledBack);
+
+      assertEquals(new Dir(testDir).readSync(), null);
+    } finally {
+      Deno.removeSync(testDir);
+    }
+  }
+});
+
+test({
+  name: "Async read returns one file at a time",
+  async fn() {
+    const testDir: string = Deno.makeTempDirSync();
+    Deno.createSync(testDir + "/foo.txt");
+    Deno.createSync(testDir + "/bar.txt");
+
+    try {
+      let secondCallback = false;
+      const dir: Dir = new Dir(testDir);
+      const firstRead: Dirent | null = await dir.read();
+      const secondRead: Dirent | null = await dir.read(
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        (err: any, secondResult: Dirent) => {
+          assert(
+            secondResult.name === "bar.txt" || secondResult.name === "foo.txt"
+          );
+          secondCallback = true;
+        }
+      );
+      const thirdRead: Dirent | null = await dir.read();
+
+      if (firstRead?.name === "foo.txt") {
+        assertEquals(secondRead?.name, "bar.txt");
+      } else if (firstRead?.name === "bar.txt") {
+        assertEquals(secondRead?.name, "foo.txt");
+      } else {
+        fail("File not found during read");
+      }
+      assert(secondCallback);
+      assert(thirdRead === null);
+    } finally {
+      Deno.removeSync(testDir, { recursive: true });
+    }
+  }
+});
+
+test({
+  name: "Sync read returns one file at a time",
+  fn() {
+    const testDir: string = Deno.makeTempDirSync();
+    Deno.createSync(testDir + "/foo.txt");
+    Deno.createSync(testDir + "/bar.txt");
+
+    try {
+      const dir: Dir = new Dir(testDir);
+      const firstRead: Dirent | null = dir.readSync();
+      const secondRead: Dirent | null = dir.readSync();
+      const thirdRead: Dirent | null = dir.readSync();
+
+      if (firstRead?.name === "foo.txt") {
+        assertEquals(secondRead?.name, "bar.txt");
+      } else if (firstRead?.name === "bar.txt") {
+        assertEquals(secondRead?.name, "foo.txt");
+      } else {
+        fail("File not found during read");
+      }
+      assert(thirdRead === null);
+    } finally {
+      Deno.removeSync(testDir, { recursive: true });
+    }
+  }
+});
+
+test({
+  name: "Async iteration over existing directory",
+  async fn() {
+    const testDir: string = Deno.makeTempDirSync();
+    Deno.createSync(testDir + "/foo.txt");
+    Deno.createSync(testDir + "/bar.txt");
+
+    try {
+      const dir: Dir = new Dir(testDir);
+      const results: Array<string | null> = [];
+
+      for await (const file of dir[Symbol.asyncIterator]()) {
+        results.push(file.name);
+      }
+
+      assert(results.length === 2);
+      assert(results.includes("foo.txt"));
+      assert(results.includes("bar.txt"));
+    } finally {
+      Deno.removeSync(testDir, { recursive: true });
+    }
+  }
+});
diff --git a/std/node/_fs/_fs_dirent.ts b/std/node/_fs/_fs_dirent.ts
new file mode 100644
index 000000000..38cd23d88
--- /dev/null
+++ b/std/node/_fs/_fs_dirent.ts
@@ -0,0 +1,41 @@
+import { notImplemented } from "../_utils.ts";
+
+export default class Dirent {
+  constructor(private entry: Deno.FileInfo) {}
+
+  isBlockDevice(): boolean {
+    return this.entry.blocks != null;
+  }
+
+  isCharacterDevice(): boolean {
+    return this.entry.blocks == null;
+  }
+
+  isDirectory(): boolean {
+    return this.entry.isDirectory();
+  }
+
+  isFIFO(): boolean {
+    notImplemented(
+      "Deno does not yet support identification of FIFO named pipes"
+    );
+    return false;
+  }
+
+  isFile(): boolean {
+    return this.entry.isFile();
+  }
+
+  isSocket(): boolean {
+    notImplemented("Deno does not yet support identification of sockets");
+    return false;
+  }
+
+  isSymbolicLink(): boolean {
+    return this.entry.isSymlink();
+  }
+
+  get name(): string | null {
+    return this.entry.name;
+  }
+}
diff --git a/std/node/_fs/_fs_dirent_test.ts b/std/node/_fs/_fs_dirent_test.ts
new file mode 100644
index 000000000..6fe0f4020
--- /dev/null
+++ b/std/node/_fs/_fs_dirent_test.ts
@@ -0,0 +1,123 @@
+const { test } = Deno;
+import { assert, assertEquals, assertThrows } from "../../testing/asserts.ts";
+import Dirent from "./_fs_dirent.ts";
+
+class FileInfoMock implements Deno.FileInfo {
+  len = -1;
+  modified = -1;
+  accessed = -1;
+  created = -1;
+  name = "";
+  dev = -1;
+  ino = -1;
+  mode = -1;
+  nlink = -1;
+  uid = -1;
+  gid = -1;
+  rdev = -1;
+  blksize = -1;
+  blocks: number | null = null;
+
+  isFileMock = false;
+  isDirectoryMock = false;
+  isSymlinkMock = false;
+
+  isFile(): boolean {
+    return this.isFileMock;
+  }
+  isDirectory(): boolean {
+    return this.isDirectoryMock;
+  }
+  isSymlink(): boolean {
+    return this.isSymlinkMock;
+  }
+}
+
+test({
+  name: "Block devices are correctly identified",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    fileInfo.blocks = 5;
+    assert(new Dirent(fileInfo).isBlockDevice());
+    assert(!new Dirent(fileInfo).isCharacterDevice());
+  }
+});
+
+test({
+  name: "Character devices are correctly identified",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    fileInfo.blocks = null;
+    assert(new Dirent(fileInfo).isCharacterDevice());
+    assert(!new Dirent(fileInfo).isBlockDevice());
+  }
+});
+
+test({
+  name: "Directories are correctly identified",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    fileInfo.isDirectoryMock = true;
+    fileInfo.isFileMock = false;
+    fileInfo.isSymlinkMock = false;
+    assert(new Dirent(fileInfo).isDirectory());
+    assert(!new Dirent(fileInfo).isFile());
+    assert(!new Dirent(fileInfo).isSymbolicLink());
+  }
+});
+
+test({
+  name: "Files are correctly identified",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    fileInfo.isDirectoryMock = false;
+    fileInfo.isFileMock = true;
+    fileInfo.isSymlinkMock = false;
+    assert(!new Dirent(fileInfo).isDirectory());
+    assert(new Dirent(fileInfo).isFile());
+    assert(!new Dirent(fileInfo).isSymbolicLink());
+  }
+});
+
+test({
+  name: "Symlinks are correctly identified",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    fileInfo.isDirectoryMock = false;
+    fileInfo.isFileMock = false;
+    fileInfo.isSymlinkMock = true;
+    assert(!new Dirent(fileInfo).isDirectory());
+    assert(!new Dirent(fileInfo).isFile());
+    assert(new Dirent(fileInfo).isSymbolicLink());
+  }
+});
+
+test({
+  name: "File name is correct",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    fileInfo.name = "my_file";
+    assertEquals(new Dirent(fileInfo).name, "my_file");
+  }
+});
+
+test({
+  name: "Socket and FIFO pipes aren't yet available",
+  fn() {
+    const fileInfo: FileInfoMock = new FileInfoMock();
+    assertThrows(
+      () => {
+        new Dirent(fileInfo).isFIFO();
+      },
+      Error,
+      "does not yet support"
+    );
+    assertThrows(
+      () => {
+        new Dirent(fileInfo).isSocket();
+      },
+      Error,
+      "does not yet support"
+    );
+  }
+});