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.

Sign up for GitHub

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

[WIP] fs: feature detection for recursive mkdir[Sync] #22302

Closed
wants to merge 9 commits into from
+98 −0
Closed

[WIP] fs: feature detection for recursive mkdir[Sync] #22302

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f9dd8fd
fs: feature detection for recursive mkdir[Sync]
MylesBorins Aug 13, 2018
84c1d45
fixup: util.features
MylesBorins Aug 13, 2018
c7c0853
fixup: arr -> obj
MylesBorins Aug 13, 2018
9cf4630
fixup: speeling
MylesBorins Aug 13, 2018
c3ee316
fixup: doc changes
MylesBorins Aug 14, 2018
7e4ab5d
fixup: move functionality to internal/util
MylesBorins Aug 14, 2018
efc9adf
fixup: add fs.promises
MylesBorins Aug 14, 2018
96ddc47
fixup: oops lint
MylesBorins Aug 14, 2018
7a7d800
fixup: I made a mistake
MylesBorins Aug 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2086,6 +2086,16 @@ fs.mkdir('/tmp/a/apple', { recursive: true }, (err) => {
});
```

`fs.mkdir` has non-enumerable non-writable property `Symbol('recursive')` that
can be used to detect if the recursive feature is available

```js
Object.getOwnPropertySymbols(fs.mkdir).map((sym) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if instead of a symbol specific to this feature we exposed a "feature" symbol? It could be reused in other methods, and would be well known, so it wouldn't require looping over properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds interesting! Are you imagining that Symbol('features') would then have an array with a list of all features? Can you share a code example of how userland feature detection would look?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that Symbol('features') would be public API, which is how users would know to access it. Then, I'd do something like:

fs.mkdir[kFeaturesSymbol] = { recursive: true };

Open to suggestions on that implementation, but I'd go with an object over an array because it's a bit more flexible to work with IMO.

From a user's perspective:

const features = `require('util').FEATURES_SYMBOL;` // also open to suggestions here.
const { mkdir } = require('fs');

if (typeof features == 'symbol' && mkdir[features].recursive) {
  // use the new thing
} else {
  // do the old thing
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a commit to take this approach, lmk what you think

return sym.toString();
}).includes('Symbol(recursive)');
// true
```

See also: mkdir(2).

## fs.mkdirSync(path[, options])
Expand All @@ -2106,6 +2116,16 @@ changes:
Synchronously creates a directory. Returns `undefined`.
This is the synchronous version of [`fs.mkdir()`][].

`fs.mkdirSync` has non-enumerable non-writable property `Symbol('recursive')`
that can be used to detect if the recursive feature is available

```js
Object.getOwnPropertySymbols(fs.mkdirSync).map((sym) => {
return sym.toString();
}).includes('Symbol(recursive)');
// true
```

See also: mkdir(2).

## fs.mkdtemp(prefix[, options], callback)
Expand Down
16 changes: 16 additions & 0 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,14 @@ function mkdir(path, options, callback) {
validateMode(mode, 'mode', 0o777), recursive, req);
}

Object.defineProperties(mkdir, {
[Symbol('recursive')]: {
value: true,
writable: false,
enumerable: false
}
});

function mkdirSync(path, options) {
if (typeof options === 'number' || typeof options === 'string') {
options = { mode: options };
Expand All @@ -766,6 +774,14 @@ function mkdirSync(path, options) {
handleErrorFromBinding(ctx);
}

Object.defineProperties(mkdirSync, {
[Symbol('recursive')]: {
value: true,
writable: false,
enumerable: false
}
});

function readdir(path, options, callback) {
callback = makeCallback(typeof options === 'function' ? options : callback);
options = getOptions(options, {});
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-fs-mkdir.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,22 @@ if (common.isMainThread && (common.isLinux || common.isOSX)) {
});
}

// mkdirp and mkdirSyncp feature detection
{
assert.strictEqual(Object.getOwnPropertySymbols(fs.mkdir).map((sym) => {
return sym.toString();
}).includes('Symbol(recursive)'), true);
assert.strictEqual(Object.getOwnPropertySymbols(fs.mkdirSync).map((sym) => {
return sym.toString();
}).includes('Symbol(recursive)'), true);
assert.strictEqual(Object.getOwnPropertySymbols(fs.mkdir).map((sym) => {
return sym.toString();
}).includes('Symbol(fhwdgads)'), false);
assert.strictEqual(Object.getOwnPropertySymbols(fs.mkdirSync).map((sym) => {
return sym.toString();
}).includes('Symbol(fhwdgads)'), false);
}

// Keep the event loop alive so the async mkdir() requests
// have a chance to run (since they don't ref the event loop).
process.nextTick(() => {});