-
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
Conversation
This commit adds a non-enumerable and non-writable property `Symbol('recursive')` to fs.mkdir and fs.mkdirSync to allow developers to feature detect How it can be used: ```js Object.getOwnPropertySymbols(fs.mkdir).map((sym) => { return sym.toString(); }).includes('Symbol(recursive)'); // true ```
I don’t think we should encourage that kind of Symbol-digging, and I don’t really see a reason for this to be non-enumerable (or non-writable, since writability might be nice for tests). What’s the motivation for making it so hard, instead of having |
doc/api/fs.md
Outdated
can be used to detect if the recursive feature is available | ||
|
||
```js | ||
Object.getOwnPropertySymbols(fs.mkdir).map((sym) => { |
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.
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.
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.
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?
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.
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
}
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.
Just pushed a commit to take this approach, lmk what you think
@addaleax I was thinking that this would be a good place to start for a discussion about how to do feature detection. The biggest reason for making this a symbol and non-enumerable rather than just an extension to the object was that this method is unlikely to break any current workflows or monkey patching of the object. This could scale across the entire code base without disrupting or extending any object in a noticeable way. While the symbol digging is a bit verbose it could easily be abstracted into a user land lib to function hasFeature(obj, feature) {
Object.getOwnPropertySymbols(obj).map((sym) => {
return sym.toString();
}).includes(`Symbol(${feature})`);
} |
+1 for well known property names ...or, we could take another look at exposing the node version in a programmatic way. i tried to do this a few times and i think it was reverted but maybe we can come up with something else. |
@devsnek it hasn't landed in a release yet. If you think that is the case we still have time to rebase it out of 10.x and land another change. That being said I disagree with the sentiment and don't feel that we need another API just for this case. Feature detection is a larger problem in core, and I personally like the idea of using symbols for features as opposed to sniffing semver or for specific bits on the object. If we follow @cjihrig's suggestion about and make a // in common
common.features = Symbol('features');
// in fs
Object.defineProperty(mkdir, common.features, {
value: ['recursive'],
writable: false,
enumerable: false
});
// in userland
fs.mkdir[common.features].includes('recursive'); |
What about |
Check for capabilities, not for versions. 👎 to any solution involving SemVer |
Quick update... I've taken @cjihrig's suggestion and made a well known symbol exposed on util and made the value an object rather than an array... for the most part this hides all the symbol bits and makes the userland code much more readable fs.mkdir[util.features].recursive;
// true @addaleax does this alleviate some of your concerns? |
--> | ||
|
||
* {symbol} that can be used to do feature detection. | ||
|
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.
😍😻😍
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.
Yes, this is definitely fine with me. 👍
doc/api/fs.md
Outdated
@@ -2086,6 +2086,14 @@ fs.mkdir('/tmp/a/apple', { recursive: true }, (err) => { | |||
}); | |||
``` | |||
|
|||
The `util.features` symbol can be used to feature detect if | |||
recusion is available. |
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.
typo: recursion
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.
I wanted to recuse myself 😅
doc/api/fs.md
Outdated
@@ -2106,6 +2114,14 @@ changes: | |||
Synchronously creates a directory. Returns `undefined`. | |||
This is the synchronous version of [`fs.mkdir()`][]. | |||
|
|||
The `util.features` symbol can be used to feature detect if | |||
recusion is available. |
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.
ditto
I've added this to the |
As @boneskull pointed out we already have Is this PR a better alternative? Possibly. I think |
I agree with that statement. |
doc/api/util.md
Outdated
@@ -179,6 +179,13 @@ The `--throw-deprecation` command line flag and `process.throwDeprecation` | |||
property take precedence over `--trace-deprecation` and | |||
`process.traceDeprecation`. | |||
|
|||
### util.features | |||
<!-- YAML | |||
added: REPlACEME |
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.
l -> L
lib/util.js
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
can the description be node.util.features
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
One drawback of this approach is that users will need to do recursive nesting checks to avoid getting errors in older Node versions. For this feature specifically, you'd have to do: util.features && fs.mkdir[util.features].recursive However, in the future, it gets even worse; if you're checking for hypothetical util.features && modulex.methody[util.features] && modulex.methody[util.features].featurez It works, but it's a pretty long-winded feature detection check IMO. |
doc/api/fs.md
Outdated
@@ -2106,6 +2114,14 @@ changes: | |||
Synchronously creates a directory. Returns `undefined`. | |||
This is the synchronous version of [`fs.mkdir()`][]. | |||
|
|||
The `util.features` symbol can be used to feature detect if |
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
?
I don't see why the first conditional is necessary. This should work fine + cache:
|
Alternative approach and less verbose
|
TBH, I'm not a fan of this approach when we could just have a separate |
@jasnell it doesn't seem scalable to me to make a new API for every new flag or feature we add to an API |
I don't like this approach. While feature detection is a good thing to have, and I am not against landing this per se, this does not solve the recursive mkdir problem. And I am against landing this as a solution to the recursive mkdir feature detection problem. The thing is -- users (ok, almost all users) won't care about complex feature detection. If they care about old versions -- they will use userland And this is what would happen on old versions (e.g. when someone would try to use old Node.js to run a module which expects new API): > fs.mkdir('./foo/sdfsd/df', { recursive: true }, (...args) => console.log(args))
undefined
> [ { Error: ENOENT: no such file or directory, mkdir './foo/sdfsd/df'
errno: -2,
code: 'ENOENT',
syscall: 'mkdir',
path: './foo/sdfsd/df' } ] This error message is completely unhelpful for the end user, and looks like there is a bug somewhere, either in their code or their deps. With > fs.mkdirp('./foo/sdfsd/df', (...args) => console.log(args))
TypeError: fs.mkdirp is not a function This is much more understandable, imo. Why not just a one-line It would be shorter, easier to support, would involve less code and doc changes, and will provide both the ability to feature-detect and a more simple way to use it while giving sensible errors to users of unsupported Node.js versions. |
@bcoe That's certainly better if we can't have a feature detection flag. I still dislike version sniffing because it's brittle; if |
ok folks, should I back mkdirp out of the v10.9.0 release going out tomorrow so we don't bake in something difficult to revert? |
@rvagg I think so. There's no rush on getting that feature into 10.9.0. If it waits until 10.10.0, that should be fine. That gives us the opportunity to consider changing the API as some are suggesting here. As I wrote in another issue, better to go slow and get this right then rush it out and get it wrong. |
Perhaps determining support for a new feature can be punted to a userland module. I've mocked up such a module here: https://github.com/bengl/core-features
If its data was consistently updated (and backfilled for previous features), it could be a useful alternative to building this functionality into core. In addition, this can work for older versions, prior to any version detection efforts. Such a module could be adopted/blessed into the |
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.
Making my objection explicit.
@@ -366,8 +366,20 @@ function isInsideNodeModules() { | |||
return false; | |||
} | |||
|
|||
const kCustomFeatureDetectionSymbol = Symbol('node.util.features'); |
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.
I think this should be a Symbol.for('node.util.features')
. It makes things a lot simpler for people maintaining isomorphic modules.
@bcoe It does just a bit, because certain features gets backported across multiple lines, which makes anything related to version checking hard. It works well just for semver-major changes, not for things like this. |
FWIW python also takes the different-function-name approach ( |
I agree with the above by @isaacs and @gabrielschulhof. Also the suggestion of an external package maintained and published by the org seems appealing to me (among other reasons, since it can be backwards compatible unlike something added to core). |
Discussed at TSC meeting. Will open a new issue to come to a decision about how we want to do feature detection and mark this one as blocked until that gets resolved. |
Yes it does.
Il giorno mer 29 ago 2018 alle 23:52 Christopher Hiller <
notifications@github.com> ha scritto:
… @Trott <https://github.com/Trott> Does this block #21875
<#21875> from being released?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22302 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AADL40Wb4Jo8yH_w-oarjB21WstUj87pks5uVw0RgaJpZM4V7dWv>
.
|
I don't think we are moving forward with this, closing. |
I understand why the options route to extend |
@sam-github This was a point raised above in #22302 (comment). Ultimately, I think the thing we may be learning is that feature detection for stuff like this isn't nearly as widely needed as we imagine. That too is a point made by others above, though. |
I'm curious where the data from that came from. I mean, if there is no way to detect certain features as a module author, I'm just going to have to publish modules that are brittle and do version sniffing instead. In the past, I felt like the Node.js project said that it was bad to version sniff the It would be awesome, though, if we're expected to version sniff, if logic such as the module |
It's anecdotal and I could certainly be drawing the wrong conclusion. A reasonable alternative conclusion would be "people aren't using the feature widely yet". The data/anecdotal information is this: We had a lot of debate and discussion about how to do feature detection, whether we should release the feature before we had that sorted out, etc. When we did release the feature without feature detection? Nothing bad seems to have happened. |
That's just because I gave up and just assumed no feature detection would ever be delivered and just went with version sniffing. Like I said, if version sniffing is the correct solution, so be it, as it's the only option I have. |
This commit adds a non-enumerable and non-writable property
Symbol('recursive')
to fs.mkdir and fs.mkdirSync to allow developersto feature detect
This PR adds a known symbol
util.features
that can be used to do feature detection on particular APIS.How it can be used:
edited: