Repository navigation
how to do feature detection #22585
Description
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.
Activity
Trott commentedon Aug 29, 2018
For
mkdirp, I'm in favor of making it its own function so users can check for existence offs.mkdirp. But that doesn't answer the question going forward because a separate function is not always sensiblejasnell commentedon Aug 29, 2018
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 commentedon Aug 30, 2018
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 commentedon Sep 1, 2018
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
semvermodule (and I'd say it would be a mistake because of howsatisfies()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 commentedon Sep 4, 2018
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 commentedon Sep 5, 2018
Ultimately, the most important thing to me is that we ship the new
fs.mkdiroption 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 commentedon Sep 12, 2018
@boneskull How would you feel about shipping the API as it is but also including an
fs.mkdirp()that is a wrapper offs.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 recursivemkdir()as well as recursivermdir()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 commentedon Sep 12, 2018
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 commentedon Sep 12, 2018
@MylesBorins What do you mean? Do you want to revert #21875?
MylesBorins commentedon Sep 12, 2018
no... I am a huge +1 on fs.mkdir with the recursive flag. I am minus one on the alias to fs.mkdirp
boneskull commentedon Sep 12, 2018
@Trott who else needs to be looped in here?
Trott commentedon Sep 12, 2018
@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()andrmdir()but probably not much (or nothing) else. A case could be made that doing it that way is paving the cowpaths of themkdirpandrimrafmodules.Trott commentedon Sep 12, 2018
I'd guess @nodejs/tsc and @nodejs/fs.
Trott commentedon Sep 12, 2018
Thinking about it more, considering
mkdirp()andrmdirp()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 notmkdirp()andrmdirp()are sensible approaches or awful abominations.21 remaining items
Trott commentedon Sep 19, 2018
@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 commentedon Sep 19, 2018
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 commentedon Sep 19, 2018
I believe @bengl has published a proof of concept module for this.
Fishrock123 commentedon Sep 19, 2018
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 commentedon Sep 19, 2018
@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 commentedon Sep 19, 2018
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 commentedon Sep 19, 2018
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 commentedon Sep 19, 2018
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 commentedon Sep 20, 2018
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 usingfs-extraormkdirpmodules 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 commentedon Sep 20, 2018
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 commentedon Sep 20, 2018
I'm happy with @Trott suggestion and agree we should just ship the mkdir update as is.
gabrielschulhof commentedon Sep 21, 2018
I second @mhdawson's words regarding @Trott's sugestion.
Trott commentedon Sep 25, 2018
I'm going to remove the
tsc-agendaand 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.