-
Notifications
You must be signed in to change notification settings - Fork 30k
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
[WIP] fs: feature detection for recursive mkdir[Sync] #22302
Changes from 4 commits
f9dd8fd
84c1d45
c7c0853
9cf4630
c3ee316
7e4ab5d
efc9adf
96ddc47
7a7d800
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,6 +179,13 @@ The `--throw-deprecation` command line flag and `process.throwDeprecation` | |
property take precedence over `--trace-deprecation` and | ||
`process.traceDeprecation`. | ||
|
||
### util.features | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: |
||
<!-- YAML | ||
added: REPlACEME | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. l -> L |
||
--> | ||
|
||
* {symbol} that can be used to do feature detection. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😍😻😍 |
||
## util.format(format[, ...args]) | ||
<!-- YAML | ||
added: v0.5.3 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ const { | |
O_SYMLINK | ||
} = constants; | ||
|
||
const { _extend } = require('util'); | ||
const { _extend, features } = require('util'); | ||
const pathModule = require('path'); | ||
const { isUint8Array } = require('internal/util/types'); | ||
const binding = process.binding('fs'); | ||
|
@@ -745,6 +745,14 @@ function mkdir(path, options, callback) { | |
validateMode(mode, 'mode', 0o777), recursive, req); | ||
} | ||
|
||
Object.defineProperty(mkdir, features, { | ||
value: { | ||
recursive: true | ||
}, | ||
writable: false, | ||
enumerable: false | ||
This comment was marked as resolved.
Sorry, something went wrong. |
||
}); | ||
|
||
function mkdirSync(path, options) { | ||
if (typeof options === 'number' || typeof options === 'string') { | ||
options = { mode: options }; | ||
|
@@ -766,6 +774,14 @@ function mkdirSync(path, options) { | |
handleErrorFromBinding(ctx); | ||
} | ||
|
||
Object.defineProperty(mkdirSync, features, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we also add this property on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that once we have consensus on this PR we should make a tracking issue and potentially get a bunch of different APIs properly setup with this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I was more specifically thinking we should have the symbol on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we get consensus on this approach, let's pull the
|
||
value: { | ||
recursive: true | ||
}, | ||
writable: false, | ||
enumerable: false | ||
}); | ||
|
||
function readdir(path, options, callback) { | ||
callback = makeCallback(typeof options === 'function' ? options : callback); | ||
options = getOptions(options, {}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1472,6 +1472,7 @@ module.exports = exports = { | |
callbackify, | ||
debuglog, | ||
deprecate, | ||
features: Symbol('features'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can the description be
This comment was marked as resolved.
Sorry, something went wrong. |
||
format, | ||
formatWithOptions, | ||
getSystemErrorName, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ const common = require('../common'); | |
const assert = require('assert'); | ||
const fs = require('fs'); | ||
const path = require('path'); | ||
const { features } = require('util'); | ||
|
||
const tmpdir = require('../common/tmpdir'); | ||
tmpdir.refresh(); | ||
|
@@ -172,6 +173,14 @@ if (common.isMainThread && (common.isLinux || common.isOSX)) { | |
}); | ||
} | ||
|
||
// mkdirp and mkdirSyncp feature detection | ||
{ | ||
assert.strictEqual(fs.mkdir[features].recursive, true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's add a test for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which undefined case? where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think it would be good to demonstrate that this will work fine for feature detection when
|
||
assert.strictEqual(fs.mkdir[features].fhwdgads, undefined); | ||
assert.strictEqual(fs.mkdirSync[features].recursive, true); | ||
assert.strictEqual(fs.mkdirSync[features].fhwdgads, undefined); | ||
} | ||
|
||
// 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(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to
util.features
?