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

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

Closed
wants to merge 9 commits into from

Conversation

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Aug 13, 2018

This commit adds a non-enumerable and non-writable property
Symbol('recursive') to fs.mkdir and fs.mkdirSync to allow developers
to 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:

fs.mkdir[util.features].recursive
// true
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
```
@MylesBorins MylesBorins mentioned this pull request Aug 13, 2018
4 of 4 tasks complete
@addaleax
Copy link
Member

@addaleax addaleax commented Aug 13, 2018

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 fs.mkdir.hasRecursive = true and fs.mkdirSync.hasRecursive = true? (edit: or something like @cjihrig’s suggestion below)

can be used to detect if the recursive feature is available

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

This comment has been minimized.

@cjihrig

cjihrig Aug 13, 2018
Contributor

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.

This comment has been minimized.

@MylesBorins

MylesBorins Aug 13, 2018
Author Member

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?

This comment has been minimized.

@cjihrig

cjihrig Aug 13, 2018
Contributor

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
}

This comment has been minimized.

@MylesBorins

MylesBorins Aug 13, 2018
Author Member

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

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Aug 13, 2018

@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})`);
}
@devsnek
Copy link
Member

@devsnek devsnek commented Aug 13, 2018

we really should have just made a separate method bleh i confused mkdirp and scandir in my head... ignore me.

+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.

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Aug 13, 2018

@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 features symbol, we could potentially share that symbol as a global which will make getting the results far less complicated

// 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');
@boneskull
Copy link
Contributor

@boneskull boneskull commented Aug 13, 2018

What about process.features or process.config? Would process.features.fs_mkdirp = true make sense?

@boneskull
Copy link
Contributor

@boneskull boneskull commented Aug 13, 2018

Check for capabilities, not for versions. 👎 to any solution involving SemVer

MylesBorins added 2 commits Aug 13, 2018
@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Aug 13, 2018

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.

This comment has been minimized.

@jdalton

jdalton Aug 13, 2018
Member

😍😻😍

Copy link
Member

@addaleax addaleax left a comment

Yes, this is definitely fine with me. 👍

@@ -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.

This comment has been minimized.

@addaleax

addaleax Aug 13, 2018
Member

typo: recursion

This comment has been minimized.

@MylesBorins

MylesBorins Aug 13, 2018
Author Member

I wanted to recuse myself 😅

@@ -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.

This comment has been minimized.

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Aug 13, 2018

I've added this to the tsc-agenda as this would set a precedent for how we would do feature detection in core.

@richardlau
Copy link
Member

@richardlau richardlau commented Aug 13, 2018

As @boneskull pointed out we already have process.features (since at least v0.10.x but probably earlier). I don't know if anyone is actually using it for feature detection (we appear to have not documented it) but that is what it appears to be for. (I can't recall without checking what's currently in there -- I think there were some openssl features (since removed?) at one point).

Is this PR a better alternative? Possibly. I think process.features makes sense for larger coarser grained features not necessarily for method level tweaks (I can imagine process.features becoming overly large if we went down this route).

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 13, 2018

I think process.features makes sense for larger coarser grained features not necessarily for method level tweaks (I can imagine process.features becoming overly large if we went down this route).

I agree with that statement.

@@ -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

This comment has been minimized.

@cjihrig

cjihrig Aug 13, 2018
Contributor

l -> L

lib/util.js Outdated
@@ -1472,6 +1472,7 @@ module.exports = exports = {
callbackify,
debuglog,
deprecate,
features: Symbol('features'),

This comment has been minimized.

@devsnek

devsnek Aug 13, 2018
Member

can the description be node.util.features

This comment has been hidden.

@MylesBorins

MylesBorins Aug 14, 2018
Author Member

I'm not sure what you mean

@RyanZim
Copy link

@RyanZim RyanZim commented Aug 14, 2018

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 modulex.methody[util.features].featurez that got added in some future release, and you have to support Node versions back before the addition of util.features, you have to do three checks:

util.features && modulex.methody[util.features] && modulex.methody[util.features].featurez

It works, but it's a pretty long-winded feature detection check IMO.

@@ -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

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 14, 2018
Member

Link to util.features?

@Fishrock123
Copy link
Member

@Fishrock123 Fishrock123 commented Aug 14, 2018

util.features && modulex.methody[util.features] && modulex.methody[util.features].featurez

I don't see why the first conditional is necessary.

This should work fine + cache:

const features = modulex.methody[util.features]
if (features && features.recursive) { }
@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Aug 14, 2018

@RyanZim

Alternative approach and less verbose

try {
  if (!fs.mkdir[util.features].recursive) throw 'woops'
  // Do a thing
}
catch (e) {
  // Polyfill
}
@jasnell
Copy link
Member

@jasnell jasnell commented Aug 14, 2018

TBH, I'm not a fan of this approach when we could just have a separate if (fs.mkdirp) to do the same thing.

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Aug 14, 2018

@jasnell it doesn't seem scalable to me to make a new API for every new flag or feature we add to an API

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Aug 14, 2018

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 mkdirp module. If they don't care about old Node.js versions -- they will just use the native impl. So they will just use the most simple way to poke the new code path, which currently is fs.mkdir(path, {recursive: true}), without even caring about feature detection stuff (who wants more code?).

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, this is what would happen:

> 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 fs.mkdirp wrap alias to fs.mkdir, as @jasnell suggests above? We already have aliases of different sorts. See previous discussion at #21875 (comment) (and response to that).

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.

@RyanZim
Copy link

@RyanZim RyanZim commented Aug 15, 2018

@bcoe That's certainly better if we can't have a feature detection flag.

I still dislike version sniffing because it's brittle; if recursive ever gets backported, libraries have to be updated to know about the backport (if there's no potential for backporting, then moot point).

@rvagg
Copy link
Member

@rvagg rvagg commented Aug 15, 2018

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?

@Trott
Copy link
Member

@Trott Trott commented Aug 15, 2018

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.

@bengl
Copy link
Member

@bengl bengl commented Aug 16, 2018

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

Note: the included data is super-minimal, and just to see this working at all. Also, there's plenty of room for it being implemented in other ways.

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 nodejs org, and updated as a part of the release process when any semver-minor/major commits are released. Certainly more process isn't always ideal, but I think this would be a pretty minimal addition.

@bcoe bcoe mentioned this pull request Aug 29, 2018
3 of 4 tasks complete
Copy link
Member

@mcollina mcollina left a comment

Making my objection explicit.

@@ -366,8 +366,20 @@ function isInsideNodeModules() {
return false;
}

const kCustomFeatureDetectionSymbol = Symbol('node.util.features');

This comment has been minimized.

@mcollina

mcollina Aug 29, 2018
Member

I think this should be a Symbol.for('node.util.features'). It makes things a lot simpler for people maintaining isomorphic modules.

@mcollina
Copy link
Member

@mcollina mcollina commented Aug 29, 2018

a nice middle-ground for making features easier to detect would be having the numeric major, minor and patch stored somewhere on process -- then the feature detection logic becomes quite a bit more straight-forward

@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.

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Aug 29, 2018

FWIW python also takes the different-function-name approach (os.makedirs). @isaacs' words about atomicity resonate with me. Do we want our APIs to be more like the UNIX shell utilities, or more like the system calls? I think fs.mkdir is a does-one-thing-and-does-it-well API. On the other hand, the name of the syscall coincides with the name of the shell utility in the UNIX world. Not so for rm (vs. unlink). I don't think that, just because the UNIX utility named mkdir offers recursive capabilities, ours must also.

@apapirovski
Copy link
Member

@apapirovski apapirovski commented Aug 29, 2018

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).

@Trott
Copy link
Member

@Trott Trott commented Aug 29, 2018

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.

@Trott Trott removed the tsc-agenda label Aug 29, 2018
@boneskull
Copy link
Contributor

@boneskull boneskull commented Aug 29, 2018

@Trott Does this block #21875 from being released?

@mcollina
Copy link
Member

@mcollina mcollina commented Aug 30, 2018

@MylesBorins
Copy link
Member Author

@MylesBorins MylesBorins commented Oct 29, 2018

I don't think we are moving forward with this, closing.

@sam-github
Copy link
Contributor

@sam-github sam-github commented Nov 2, 2018

I understand why the options route to extend fs.mkdir() was attractive, but note that if this had been fs.mkdirp() the feature detection would have been if (fs.mkdirp) ...

@Trott
Copy link
Member

@Trott Trott commented Nov 3, 2018

@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.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Nov 3, 2018

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.

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 process.version, but if that is the expected behavior to do in code, then I can definately change.

It would be awesome, though, if we're expected to version sniff, if logic such as the module semver was actually in core so the version sniffing is more robust than process.version.split('.').

@Trott
Copy link
Member

@Trott Trott commented Nov 3, 2018

I'm curious where the data from that came from.

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.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Nov 3, 2018

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.

@Trott Trott mentioned this pull request Aug 24, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.