how to do feature detection

[Trott]

Ref: #22302

We need to make a decision about how to do feature detection so we can land the mkdirp implementation in a release. (Right now, it's in master, but no releases.)

@nodejs/tsc

Sorry this is a stub right now, but feel free to add information so I don't have to come back and do it later.

[Trott]

For mkdirp, I'm in favor of making it its own function so users can check for existence of fs.mkdirp. But that doesn't answer the question going forward because a separate function is not always sensible

[jasnell]

separate function is not always sensible

I think this is a more important distinction that it may first appear. Are there reasonable general heuristics we can apply to this choice?

[bengl]

I still think having an external module to tell whether a feature is available or not is the best approach, because it can potentially be used on older versions of Node without much issue. In addition, keeping the functionality external frees us to do whatever is most sensible when designing new APIs or adding functionality to existing APIs.

See my comment on the other thread, where I mocked up a module that would work like this.

[targos]

I also think it's better to keep this outside of core. The best way to know anything about a feature (is it here? is this new option available? has the behavior changed in this case?) is to look at the history in the documentation and compare with the current version number.
It's not necessary to pull in the semver module (and I'd say it would be a mistake because of how satisfies() handles pre-releases). What we can do to help a "core-features" module (and probably other packages that already depend on semver just to check the major/minor/patch numbers) is to expose the individual numbers in separate properties.

[boneskull]

As I mentioned in the other issue, userland libs will be where most of the version/feature detection happens.

Userland can perform feature detection using whatever method it deems best.

But reading docs across multiple release lines--manually sifting back through old versions-- to determine when a feature appeared seems painful.

Node.js can help userland make those decisions by providing a single reference document showing in which version a feature or significant API change appeared.

[boneskull]

Ultimately, the most important thing to me is that we ship the new fs.mkdir option as submitted, because this is the most natural API.

IMO, feature detection isn't a convincing reason to use fs.mkdirp. That's just punting on a feature detection strategy.

[Trott]

@boneskull How would you feel about shipping the API as it is but also including an fs.mkdirp() that is a wrapper of fs.mkdir() that sets the recursive option? I'm not a fan of creating multiple ways to do the same thing, but there's precedence for that all over JavaScript and it does provide simple feature detection for recursive mkdir() as well as recursive rmdir() if we do something similar there.

We still have to decide if feature detection should be done the way @MylesBorins proposes or the way @bengl proposes or both or some other way. But it does free us to move forward with releasing recursive mkdir() while we figure that out.

[MylesBorins]

I'm sorry for being difficult but I am -1 on including mkdirp, specifically I find adding a new api for recursive apis to be untenable and unscalable

[targos]

@MylesBorins What do you mean? Do you want to revert #21875?

[MylesBorins]

no... I am a huge +1 on fs.mkdir with the recursive flag. I am minus one on the alias to fs.mkdirp

[boneskull]

@Trott who else needs to be looped in here?

[Trott]

I'm sorry for being difficult but I am -1 on including mkdirp, specifically I find adding a new api for recursive apis to be untenable and unscalable

@MylesBorins That's not being difficult. That's just expressing your opinion. Which is what we're looking for in this issue.

I'm not sure what you mean by untenable in this context, but unscalable is very clear. I think it might make sense for recursive mkdir() and rmdir() but probably not much (or nothing) else. A case could be made that doing it that way is paving the cowpaths of the mkdirp and rimraf modules.

[Trott]

@Trott who else needs to be looped in here?

I'd guess @nodejs/tsc and @nodejs/fs.

[Trott]

Thinking about it more, considering mkdirp() and rmdirp() doesn't really make much sense without first resolving the feature detection story. Once that is decided, it should hopefully fall into place quickly whether or not mkdirp() and rmdirp() are sensible approaches or awful abominations.

[Trott]

I gotta step away, but if someone wants to take the time to spell out the two competing options (and any other options that I might have missed being proposed), that would be very helpful. One approach is in #22302 (proposed by @MylesBorins) and the other is the external module thing proposed by @bengl. If those could be summarized in a single comment, that would probably facilitate conversation. And if there are any other viable approaches being proposed that I've missed, someone say something!

[bengl]

As requested by @Trott, here's a summary of our options (that I'm aware of) for feature detection, in no particular order (I do have opinions on these but I'm reserving them, since I just wanted to summarize in this comment for the purposes of discussion, rather than opine):

• Option A: Add a feature detection API to Node. This is implemented in [WIP] fs: feature detection for recursive mkdir[Sync] #22302, but alternative APIs are also possible. This means we'd have an extra Symbol property on some APIs referring to an object with flags for given features.
  • e.g. fs.mkdir[util.features].recursive
• Option B: Add new (or alias) APIs where feature detection would otherwise be impractical. This has been mentioned in the original PR for recursive mkdir and elsewhere. fs.mkdirp is the current canonical example. Note that these could also be aliases for non-separate APIs.
  • e.g. fs.mkdirp
• Option C: Use an external library for feature detection. This was first mentioned here. This would have an external module as the officially recommended way to detect features.
  • e.g. require('core-features')('fs.mkdir.recursive')
• Option D: Do nothing, and depend only on version numbers for feature detection. This is the current status quo, and how many libraries already decide whether or not they can use certain features or not.
  • e.g. Number(process.versions.node.split('.')[1]) >= 10 // etc...

[jasnell]

In order of preference for me: B, D, C, A

…

On Wed, Sep 12, 2018, 21:47 Bryan English ***@***.***> wrote: As requested by @Trott <https://github.com/Trott>, here's a summary of our options (that I'm aware of) for feature detection, in no particular order (I do have opinions on these but I'm reserving them, since I just wanted to summarize in this comment for the purposes of discussion, rather than opine): - *Option A: Add a feature detection API to Node.* This is implemented in #22302 <#22302>, but alternative APIs are also possible. This means we'd have an extra Symbol property on some APIs referring to an object with flags for given features. - e.g. fs.mkdir[util.features].recursive - *Option B: Add new (or alias) APIs where feature detection would otherwise be impractical.* This has been mentioned in the original PR for recursive mkdir <#21875> and elsewhere. fs.mkdirp is the current canonical example. Note that these could also be aliases for non-separate APIs. - e.g. fs.mkdirp - *Option C: Use an external library for feature detection.* This was first mentioned here <#22302 (comment)>. This would have an external module as the officially recommended way to detect features. - e.g. require('core-features')('fs.mkdir.recursive') - *Option D: Do nothing, and depend only on version numbers for feature detection.* This is the current status quo, and how many libraries already decide whether or not they can use certain features or not. - e.g. Number(process.versions.node.split('.')[1]) >= 10 // etc... — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#22585 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2edf2KY5jALJyBOGogRgWXuzOdMPBks5uacc4gaJpZM4WR5H2> .

[bengl]

@jasnell Any thoughts on why that ordering?

[jasnell]

B is generally easiest for users. Want thing? Check for existence of thing. Yes, there's an API surface area management issue but that's the same API surface area management issue that's always existed and it forces us to be diligent and deliberate about what we add. In other words, status quo.

D ... Also generally status quo.

C is not ideal because it adds an out of band dependency that must be managed, updated, etc. It's more cognitive overhead for users and can quickly get out of sync.

A also adds to users cognitive overhead. Need thing? Need to know feature identifier for thing, need to know if this version of node.js has util.features at all, need to know what thing the util.features is hung off of, etc.

Further, A only handles cases of adding or changing features of existing apis. It is pointless for new API. So users would still be doing B anyway for new things.

[targos]

My order of preference is: D, C, A, B.

I'm strongly against B because it is impossible to do it for every API addition and I don't think there are features that deserve more to be detected than others.

[joyeecheung]

My preference is: B, C~D, A

I think in practice C is about the same as D since it's likely that the user would rather use a library like semver to compare versions - though in a even more verbose manner.
If I remember correctly B has always been a preferred way to do feature detection on the Web i.e. a big part of the JS ecosystem. Even if we go with A, developers still have to do feature detection for the feature detection API itself, or resort to C/D to know if the feature detection API is available, which only seems to complicate matters.

[mcollina]

My preferences: B, D, A.

We should do C anyway. readable-stream@2 accumulated such a long list of "B" approach that was staggering. I think it would be good and healthy for the ecosystem.

[bengl]

My order of preference is C, D, A, B.

Node.js isn't the web, and solutions in either platform don't have to be the same. In Node.js, we're in a situation where the version number is exposed programmatically, so as long as the API is documented for each version, it's a straightforward jump to knowing exactly what you can and can't do with each API function.

I don't think it's crucial that this be streamlined, because people have been checking version numbers of Node.js programatically to see what they can and can't do for many years now. This is normal usage of Node.js when supporting multiple versions. That being said, it's probably best for Node.js' users if something is provided to make that easier, so that's why I'm a fan of the C approach, with D as a very close second.

Adding an extra property on certain functions in order to check what features they support seems somewhat reasonable, but would rely on detecting the availability of such things through other means, like by version number or detecting the presence of util.features, so it actually results in the same issue.

I think unless you're on a platform where you don't have a versioned API, it doesn't make sense to build the API holding feature detection at a higher priority than clean and clear API design. When adding functionality that is a variation on an existing function, the natural thing to do is add it as an option to the original function. Adding a separate (or alias) API function for something that does effectively the same thing but with extras is confusing for users, and we already see that in some older Node.js APIs (in practice, I've seen child_process trip up even well-seasoned Node.js developers). In the fs.mkdir case, the function is already name tersely, and adding p to the end makes it even more unclear for folks who have never used either the mkdirp package or mkdir -p on a UNIX system. I'd rather not confuse users for the purposes of feature detection, so I'm strongly against B.

---

That all being said, I agree with @mcollina that regardless of the approach decided upon, C and and should be done regardless, since there are already many APIs in Node.js where this would be very helpful to people.

[danbev]

My preferences are: B, D, C, A

[BridgeAR]

My preferences are: B, D, C, A

[tniessen]

How would B work for other APIs? For example, #14731 added an option verbatim to dns.lookup, #20235 and #20039 added a (security-relevant!) authTagLength option to crypto.createCipher*, and there are many more examples. It doesn't seem reasonable to add API aliases for all of these.

[gabrielschulhof]

@tniessen IIUC B is supposed to be for situations where it's "impractical" to do feature detection. That's a bit subjective, and since the version number is always available, I can't imagine a situation where a feature would not coincide with an interval of version numbers. I guess the version number juggling can be tricky, but as to whether it's "impractical" depends on the implementer. So, we wouldn't be adding aliases for every single new variation, but only for those that are "impractical" to detect otherwise, where the criterion of "impractical" is something we'd have to quantify.

[targos]

Other examples of API additions that cannot be easily detected:

• fs.readdir "withFileTypes": fs: readdir optionally returning type information #22020
• TypedArray support in various APIs like fs or child_process: fs: work with any TypedArrays instead of only Uint8Arrays #22150 child_process: allow typed arrays for input #22409
• Allow url and options to be passed to http.request and http.get: http: allow url and options to be passed to http.request #21616

This issue is not just about mkdirp. B might end up being a solution for that specific case (even though I don't think it should).

This issue is about finding a general programmatical answer to the question "can I use this API in the current environment?". Whatever solution we decide to take, I think it has to be applicable to a majority of the cases without getting in the way of a clean and understandable API.

[mhdawson]

My preferences are D, C, B, A with much of the same rationale as @bengl. It's hard to decide between D and C.

[jasnell]

If we adopt option A, there are a few details that I'd like to see discussed in detail before we moved forward, not because they are edge cases, but because they are realities of API design and maintenance...

1. Versioning... Let's say, for instance, in the future we make a hypothetical modification to the arguments and options for recursive mkdir (or any other new API). How would we signal the differences in those implementations? Would that be two different feature identifiers (e.g. fs.mkdir[util.features].recursive and fs.mkdir[util.features].recursive_v2)?

2. Deprecation... Let's say we have to runtime deprecate recursive mkdir in the future for any reason? Would we turn the fs.mkdir[util.features].recursive into a deprecated getter? Such that accessing fs.mkdir[util.features].recursive would emit a deprecation warning?

3. Discoverability... How will users discover which APIs have these feature tags? I assume there would be something added to the documentation, similar to how we document error codes and deprecation codes, but I haven't yet seen this discussed. I also assume that the feature labels are unique only to the thing they are attached to.

4. Scalability... Are we reasonably certain that this is a scalable and generalizable approach? By that I mean, can we apply this consistently for all feature additions? Does there need to be a rule for all semver-minor feature additions that a util.features tag is added? Or will it be necessary to do feature detection differently based on the kind of feature that is added? For instance, if I add a brand new function to the fs module, should I also add fs[util.features].brandNewFunction? If I add a new text encoding that can be used throughout Node.js, should I add a ...[util.features].myNewEncoding tag somewhere to indicate that it is supported?

Bottom line: if we are to add the util.features type discoverability, there must be clear guidelines on how it is used and how it should not be used, otherwise we will end up with problems. For precedent, when both error codes and deprecation codes were added to Node.js, they each included documented guidelines on how they were to be used.

[joyeecheung]

BTW in addition to ABCD mentioned above, what about process.features? That is a long-existing API for feature detection. If I am going to decide between process.features and B, I'd go for process.features. If we somehow decide to create another new API for feature detection, what are we going to do with process.features?

[jasnell]

process.features is a bit problematic given that:

1. It's been there for years and very very very few people actually know about it.
2. The majority of the properties currently in there are largely obsolete.
3. The process.features property (and the current objects set of properties) are configurable and writable by user code making it largely unreliable. We would have to deprecate the current behavior before we could make changes necessary to make it more reliable.
4. It would be a global namespace which means unique names for every new feature (which isn't a problem, it just adds a maintenance burden)... e.g. rather than .recursive it would need to be something like .fs_mkdir_recursive

[addaleax]

The process.features property (and the current objects set of properties) are configurable and writable by user code making it largely unreliable. We would have to deprecate the current behavior before we could make changes necessary to make it more reliable.

The flexibility of JS is a strength, and I do think it’s important that users can modify whatever solution we come up with programmatically; overwriting is not something that typically happens by accident, and doing so can be useful for testing different situations without actually having to check all existing Node.js versions.

[ljharb]

Version number detection would be just as brittle and terrible as user agent detection in browsers - whatever you decide, please come up with a way to do feature detection - ie, to test for the desired presence or behavior directly, rather than inferring it from unrelated information. (B, A, C, D)

[Trott]

My preferences are: B, D, C, A

@BridgeAR Can you elaborate? This isn't a vote. It's a discussion. So the reasons for your ordering may be more important than the ordering itself.

[Fishrock123]

B seems completely impractical long-term to me. It just does not cover many access well outside of the feature in question.

That being said C, is possible regardless of what we decide. Someone could already have made that. Honestly, if no one has already made that yet, is feature detection really that important? 🤔

[Trott]

That being said C, is possible regardless of what we decide. Someone could already have made that. Honestly, if no one has already made that yet, is feature detection really that important? 🤔

I believe @bengl has published a proof of concept module for this.

[Fishrock123]

I believe Bryan English has published a proof of concept module for this.

Do a significant amount of people find it useful & use it? I'm looking for more concrete evidence in the Node.js ecosystem here.

[bengl]

@Fishrock123 No, because it's currently just a scaffold/proof-of-concept/not-even-published on npm yet: https://github.com/bengl/core-features

I put it together on a suggestion from @MylesBorins, in order to see what this might look like. Such a module could (but certainly not necessarily) be adopted by Node and baked into Node's release process to ensure freshness.

For comparison in the wild, we can look at browsers, where people have used https://modernizr.com/ for years. In Node-land, what I've seen (personally, anecdotally) is a lot of checking of version numbers.

[Fishrock123]

For comparison in the wild, we can look at browsers, where people have used https://modernizr.com/ for years. In Node-land, what I've seen (personally, anecdotally) is a lot of checking of version numbers.

This all misses the point. My thought is, If people really wanted this, It is easy enough to do in userland that it would already be done...

That it seems like it's completely non-existent makes me wonder if this just isn't all that necessary for Node.js.

[ofrobots]

Given options A, B, C, D: one of these things is not like the other.

Options A, C, D are different mechanisms of enabling the ecosystem to determine whether a feature may be present in a given version of Node. It is not even clear to me whether these options are in competition with each other. Today, I do feature tests using version numbers (option D) – after all, our API docs do spell out when specific features were added. It may not be the most ergonomic way of doing things. Having a user space facade on top of this (option C) – as @Fishrock123 points out – is possible today, but it clearly must not be a big pain point if such a module doesn't exist. Having said that, I would not be opposed to defining a formal mechanism to expose features (option A).

Option B has the premise that if feature detection is /not practical/, we would want to add an alias. I don't believe this premise is true. Option D exists for this purpose today. Whether or not it is ergonomic enough is debatable, and either A or C could address this.

So with that in mind: I favour option D. I would be supportive of implementing A. Option C is about an ecosystem module that could be implemented at any time if it was necessary so our opinion doesn't really matter.

I am opposed to option B – we should try to avoid creating aliases just for the purpose of 'feature detection'.

[Fishrock123]

Having a user space facade on top of this (option C) – as Jeremiah Senkpiel points out – is possible today, but it clearly must not be a big pain point if such a module doesn't exist. Having said that, I would not be opposed to defining a formal mechanism to expose features (option A).

To me this point is kind of the opposite - why put it into core faster if nobody already needs it enough to do something relatively simple in userland? Putting a module out there for people who think they need this and actually seeing usage seems like a good way to prove that option A would be valuable.

[Trott]

I know this isn't the answer people want, but:

We will need to figure the feature detection story individually for each API on an ad hoc basis.

For some APIs, feature detection just won't be that important.

For other APIs, it may be important but not important enough to compromise on otherwise high-quality API design or increase core support burden.

And perhaps for other APIs, feature detection is critical and we may have to compromise on ideals in other areas to accommodate it.

Thinking about this more, I think it makes sense to trust that people who need extensive feature detection will rely on an external module for it. (And if it isn't needed, then the module doesn't need to exist, or it can exist and nobody uses it. But the point is that it's not a support burden for us because we're not bundling it into core.)

On the recursive mkdir thing, I propose that we land the feature in v10.x as it currently exists (an options flag on mkdir()). Some have made a (persuasive to me) case that feature detection for it is not actually a widespread need. If it comes to light that feature detection for it is a big need after all, we can add an alias .mkdirp() function. But I've become convinced that certain folks are correct when they argue "most people won't need feature detection for this, as they'll just keep on using fs-extra or mkdirp modules which will be the only place where feature detection has to happen and they can do version sniffing".

I'd love to get the @nodejs/tsc take on this approach. I know we don't want to say "sorry, can't make a decision, will have to decide ad hoc each time". But I think that really is the case, and we're not doing anyone a service by dragging the conversation out before we finally have to concede that anyway. It's not like the people who endorse option B above are going to become convinced that we should avoid option B, or vice versa.

[RyanZim]

Although I would have preferred explicit feature detection in core, I think @Trott's proposal is a sensible compromise I can live with in fs-extra.

[mhdawson]

I'm happy with @Trott suggestion and agree we should just ship the mkdir update as is.

[gabrielschulhof]

I second @mhdawson's words regarding @Trott's sugestion.

[Trott]

I'm going to remove the tsc-agenda and conclude that we will continue with status quo which is determine feature detection on a case-by-case basis. Feel free to re-open if you think that's the wrong conclusion and you're willing to do the work to drive to a different consensus.