Repository: denoland/deno
Pull request: fix(ext/node): fs.symlink and fs.symlinkSync compatibility

`node:fs.symlink` differs from `Deno.symlink` in that on Windows, Node's implementation will default the symlink type to `file` if the target doesn't exists, while Deno will throw an error. This means that it will deliberately create a broken symlink (see https://github.com/nodejs/node/blob/591ba692bfe30408e6a67397e7d18bfa1b9c3561/test/parallel/test-fs-symlink.js#L55-L63).

Allows this tests to pass:
 - parallel/test-fs-symlink-buffer-path.js
 - parallel/test-fs-symlink-dir-junction-relative.js
 - parallel/test-fs-symlink-dir.js
 - parallel/test-fs-symlink.js

Changed files:
filename: ext/node/polyfills/_fs/_fs_symlink.ts
patch: @@ -2,62 +2,110 @@
 
 import { primordials } from "ext:core/mod.js";
 
-import { CallbackWithError } from "ext:deno_node/_fs/_fs_common.ts";
-import { pathFromURL } from "ext:deno_web/00_infra.js";
+import {
+  CallbackWithError,
+  makeCallback,
+} from "ext:deno_node/_fs/_fs_common.ts";
 import { promisify } from "ext:deno_node/internal/util.mjs";
+import type { Buffer } from "node:buffer";
+import { validateOneOf } from "ext:deno_node/internal/validators.mjs";
+import { getValidatedPathToString } from "ext:deno_node/internal/fs/utils.mjs";
+import * as pathModule from "node:path";
+import { isWindows } from "ext:deno_node/_util/os.ts";
+import { stat, statSync } from "ext:deno_node/_fs/_fs_stat.ts";
 
-const {
-  Error,
-  ObjectPrototypeIsPrototypeOf,
-  PromisePrototypeThen,
-} = primordials;
+const { PromisePrototypeThen } = primordials;
 
-type SymlinkType = "file" | "dir" | "junction";
+export type SymlinkType = "file" | "dir" | "junction";
 
 export function symlink(
-  target: string | URL,
-  path: string | URL,
-  typeOrCallback: SymlinkType | CallbackWithError,
-  maybeCallback?: CallbackWithError,
+  target: string | Buffer | URL,
+  path: string | Buffer | URL,
+  linkType?: SymlinkType | CallbackWithError,
+  callback?: CallbackWithError,
 ) {
-  target = ObjectPrototypeIsPrototypeOf(URL, target)
-    ? pathFromURL(target)
-    : target;
-  path = ObjectPrototypeIsPrototypeOf(URL, path) ? pathFromURL(path) : path;
+  if (callback === undefined) {
+    callback = linkType as CallbackWithError;
+    linkType = undefined;
+  } else {
+    validateOneOf(linkType, "type", [
+      "dir",
+      "file",
+      "junction",
+      null,
+      undefined,
+    ]);
+  }
 
-  const type: SymlinkType = typeof typeOrCallback === "string"
-    ? typeOrCallback
-    : "file";
+  callback = makeCallback(callback);
+  target = getValidatedPathToString(target, "target");
+  path = getValidatedPathToString(path);
 
-  const callback: CallbackWithError = typeof typeOrCallback === "function"
-    ? typeOrCallback
-    : (maybeCallback as CallbackWithError);
+  if (isWindows && !linkType) {
+    let absoluteTarget;
+    try {
+      // Symlinks targets can be relative to the newly created path.
+      // Calculate absolute file name of the symlink target, and check
+      // if it is a directory. Ignore resolve error to keep symlink
+      // errors consistent between platforms if invalid path is
+      // provided.
+      absoluteTarget = pathModule.resolve(path, "..", target);
+    } catch {
+      // Continue regardless of error.
+    }
+    if (absoluteTarget !== undefined) {
+      stat(absoluteTarget, (err, stat) => {
+        const resolvedType = !err && stat.isDirectory() ? "dir" : "file";
 
-  if (!callback) throw new Error("No callback function supplied");
+        PromisePrototypeThen(
+          Deno.symlink(
+            target,
+            path,
+            { type: resolvedType },
+          ),
+          () => callback(null),
+          callback,
+        );
+      });
+      return;
+    }
+  }
 
   PromisePrototypeThen(
-    Deno.symlink(target, path, { type }),
+    Deno.symlink(
+      target,
+      path,
+      { type: linkType ?? "file" },
+    ),
     () => callback(null),
     callback,
   );
 }
 
 export const symlinkPromise = promisify(symlink) as (
-  target: string | URL,
-  path: string | URL,
+  target: string | Buffer | URL,
+  path: string | Buffer | URL,
   type?: SymlinkType,
 ) => Promise<void>;
 
 export function symlinkSync(
-  target: string | URL,
-  path: string | URL,
+  target: string | Buffer | URL,
+  path: string | Buffer | URL,
   type?: SymlinkType,
 ) {
-  target = ObjectPrototypeIsPrototypeOf(URL, target)
-    ? pathFromURL(target)
-    : target;
-  path = ObjectPrototypeIsPrototypeOf(URL, path) ? pathFromURL(path) : path;
-  type = type || "file";
+  validateOneOf(type, "type", ["dir", "file", "junction", null, undefined]);
+  target = getValidatedPathToString(target, "target");
+  path = getValidatedPathToString(path);
 
-  Deno.symlinkSync(target, path, { type });
+  if (isWindows && !type) {
+    const absoluteTarget = pathModule.resolve(path, "..", target);
+    if (
+      statSync(absoluteTarget, { bigint: false, throwIfNoEntry: false })
+        ?.isDirectory()
+    ) {
+      type = "dir";
+    }
+  }
+
+  Deno.symlinkSync(target, path, { type: type ?? "file" });
 }

filename: ext/node/polyfills/internal/validators.mjs
patch: @@ -210,27 +210,22 @@ function validateBoolean(value, name) {
   }
 }
 
-/**
- * @param {unknown} value
- * @param {string} name
- * @param {unknown[]} oneOf
- */
-const validateOneOf = hideStackFrames(
-  (value, name, oneOf) => {
-    if (!ArrayPrototypeIncludes(oneOf, value)) {
-      const allowed = ArrayPrototypeJoin(
-        ArrayPrototypeMap(
-          oneOf,
-          (v) => (typeof v === "string" ? `'${v}'` : String(v)),
-        ),
-        ", ",
-      );
-      const reason = "must be one of: " + allowed;
+/** @typedef {<T>(value: unknown, name: string, oneOf: readonly T[]) => asserts value is T} ValidateOneOf */
+/** @type {ValidateOneOf} */
+const validateOneOf = hideStackFrames((value, name, oneOf) => {
+  if (!ArrayPrototypeIncludes(oneOf, value)) {
+    const allowed = ArrayPrototypeJoin(
+      ArrayPrototypeMap(
+        oneOf,
+        (v) => (typeof v === "string" ? `'${v}'` : String(v)),
+      ),
+      ", ",
+    );
+    const reason = "must be one of: " + allowed;
 
-      throw new codes.ERR_INVALID_ARG_VALUE(name, value, reason);
-    }
-  },
-);
+    throw new codes.ERR_INVALID_ARG_VALUE(name, value, reason);
+  }
+});
 
 export function validateEncoding(data, encoding) {
   const normalizedEncoding = normalizeEncoding(encoding);

filename: tests/node_compat/config.toml
patch: @@ -474,7 +474,11 @@
 "parallel/test-fs-rmdir-type-check.js" = {}
 "parallel/test-fs-stream-fs-options.js" = {}
 "parallel/test-fs-stream-options.js" = {}
+"parallel/test-fs-symlink-buffer-path.js" = {}
 "parallel/test-fs-symlink-dir-junction.js" = {}
+"parallel/test-fs-symlink-dir-junction-relative.js" = {}
+"parallel/test-fs-symlink-dir.js" = {}
+"parallel/test-fs-symlink.js" = {}
 "parallel/test-fs-unlink-type-check.js" = {}
 "parallel/test-fs-util-validateoffsetlength.js" = {}
 "parallel/test-fs-write-negativeoffset.js" = {}

filename: tests/unit_node/_fs/_fs_symlink_test.ts
patch: @@ -11,7 +11,7 @@ Deno.test({
         symlink("some/path", "some/other/path", "dir");
       },
       Error,
-      "No callback function supplied",
+      "The \"cb\" argument must be of type function. Received type string ('dir')",
     );
   },
 });

filename: tools/core_import_map.json
patch: @@ -35,6 +35,7 @@
     "ext:deno_node/_fs/_fs_read.ts": "../ext/node/polyfills/_fs/_fs_read.ts",
     "ext:deno_node/_fs/_fs_readv.ts": "../ext/node/polyfills/_fs/_fs_readv.ts",
     "ext:deno_node/_fs/_fs_stat.ts": "../ext/node/polyfills/_fs/_fs_stat.ts",
+    "ext:deno_node/_fs/_fs_symlink.ts": "../ext/node/polyfills/_fs/_fs_symlink.ts",
     "ext:deno_node/_fs/_fs_watch.ts": "../ext/node/polyfills/_fs/_fs_watch.ts",
     "ext:deno_node/_fs/_fs_write.mjs": "../ext/node/polyfills/_fs/_fs_write.mjs",
     "ext:deno_node/_fs/_fs_writev.ts": "../ext/node/polyfills/_fs/_fs_writev.ts",