Skip to content
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

fs: add directory autodetection to fsPromises.symlink() #42894

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 12 additions & 3 deletions doc/api/fs.md
Expand Up @@ -1463,18 +1463,27 @@ changes:

<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42894
description: If the `type` argument is `null` or omitted, Node.js will
autodetect `target` type and automatically
select `dir` or `file`.

-->

* `target` {string|Buffer|URL}
* `path` {string|Buffer|URL}
* `type` {string} **Default:** `'file'`
* `type` {string|null} **Default:** `null`
* Returns: {Promise} Fulfills with `undefined` upon success.

Creates a symbolic link.

The `type` argument is only used on Windows platforms and can be one of `'dir'`,
`'file'`, or `'junction'`. Windows junction points require the destination path
to be absolute. When using `'junction'`, the `target` argument will
`'file'`, or `'junction'`. If the `type` argument is not a string, Node.js will
autodetect `target` type and use `'file'` or `'dir'`. If the `target` does not
exist, `'file'` will be used. Windows junction points require the destination
path to be absolute. When using `'junction'`, the `target` argument will
automatically be normalized to absolute path.

### `fsPromises.truncate(path[, len])`
Expand Down
13 changes: 12 additions & 1 deletion lib/internal/fs/promises.js
Expand Up @@ -116,6 +116,8 @@ const {
const getDirectoryEntriesPromise = promisify(getDirents);
const validateRmOptionsPromise = promisify(validateRmOptions);

const isWindows = process.platform === 'win32';

let cpPromises;
function lazyLoadCpPromises() {
return cpPromises ??= require('internal/fs/cp/cp').cpFn;
Expand Down Expand Up @@ -710,7 +712,16 @@ async function readlink(path, options) {
}

async function symlink(target, path, type_) {
const type = (typeof type_ === 'string' ? type_ : null);
let type = (typeof type_ === 'string' ? type_ : null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is semver-major anyway, we might as well use validateString here, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Something like validateOneOf(type, 'type', ['dir', 'file', 'junction', undefined]) would be perfect.
I'd also prefer to have this validation performed on all platforms for consistency.
However, technically it can break userland code (e.g. if someone used null or false), so full deprecation cycle might be safer.

if (isWindows && type === null) {
try {
const absoluteTarget = pathModule.resolve(`${path}`, '..', `${target}`);
type = (await stat(absoluteTarget)).isDirectory() ? 'dir' : 'file';
} catch {
// Default to 'file' if path is invalid or file does not exist
type = 'file';
}
}
target = getValidatedPath(target, 'target');
path = getValidatedPath(path);
return binding.symlink(preprocessSymlinkDestination(target, type, path),
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-fs-symlink-dir.js
Expand Up @@ -12,6 +12,7 @@ if (!common.canCreateSymLink())
const assert = require('assert');
const path = require('path');
const fs = require('fs');
const fsPromises = fs.promises;

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
Expand All @@ -36,11 +37,18 @@ function testAsync(target, path) {
}));
}

async function testPromises(target, path) {
await fsPromises.symlink(target, path);
fs.readdirSync(path);
}

for (const linkTarget of linkTargets) {
fs.mkdirSync(path.resolve(tmpdir.path, linkTarget));
for (const linkPath of linkPaths) {
testSync(linkTarget, `${linkPath}-${path.basename(linkTarget)}-sync`);
testAsync(linkTarget, `${linkPath}-${path.basename(linkTarget)}-async`);
testPromises(linkTarget, `${linkPath}-${path.basename(linkTarget)}-promises`)
.then(common.mustCall());
}
}

Expand All @@ -57,10 +65,17 @@ for (const linkTarget of linkTargets) {
}));
}

async function testPromises(target, path) {
await fsPromises.symlink(target, path);
assert(!fs.existsSync(path));
}

for (const linkTarget of linkTargets.map((p) => p + '-broken')) {
for (const linkPath of linkPaths) {
testSync(linkTarget, `${linkPath}-${path.basename(linkTarget)}-sync`);
testAsync(linkTarget, `${linkPath}-${path.basename(linkTarget)}-async`);
testPromises(linkTarget, `${linkPath}-${path.basename(linkTarget)}-promises`)
.then(common.mustCall());
}
}
}