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

process.features: deprecate it or add more properties? #25343

Closed
joyeecheung opened this issue Jan 4, 2019 · 5 comments
Closed

process.features: deprecate it or add more properties? #25343

joyeecheung opened this issue Jan 4, 2019 · 5 comments
Labels
discuss Issues opened for discussions and feedbacks. process Issues and PRs related to the process subsystem.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Jan 4, 2019

Stem from #25291 (comment)

What I liked about process.config.variables.v8_enable_inspector is that it's also a user accessible way to detect if the inspector is enabled. Instead of moving to an internal solution it would be rad to expose this in a more user-friendly way.

Right now, there is an undocumented process.features for that purpose, but it has not been well-maintained. It's possible to detect certain features known at build time via process.config.variables but that object is mutable in the user land so it's not exactly reliable (whereas process.features have been immutable). Also it's not really a good design to have users rely on internal variable names.

Should we deprecate it in favor of a better feature detection API, or start maintain it properly?

Related: #22585 but that is more about availability of specific APIs like recursive fs.mkdir, whereas process.features have been more about higher-level feature sets.

@joyeecheung joyeecheung added discuss Issues opened for discussions and feedbacks. process Issues and PRs related to the process subsystem. labels Jan 4, 2019
@addaleax
Copy link
Member

addaleax commented Jan 4, 2019

but that object is mutable in the user land so it's not exactly reliable (whereas process.features have been immutable).

Mutability is good – I’d be in favour of making process.features mutable, too – at the very least, that’s helpful for testing purposes, and accidental changes are incredibly unlikely.

I’d be 👍 on documenting this feature, esp. since I’d assume we can’t really remove it.

@joyeecheung joyeecheung changed the title process.features: deprecate it or adding more properties? process.features: deprecate it or add more properties? Jan 4, 2019
@joyeecheung
Copy link
Member Author

@addaleax I've took another look, the current behavior and the behavior before #25239 have both been that process.features itself is not writable/configurable while the properties on it are (i.e. the object is not frozen).

@addaleax
Copy link
Member

addaleax commented Jan 4, 2019

@joyeecheung Okay, sorry for not double-checking myself – then I’m totally fine with documenting process.features and otherwise leaving it as it is. :)

joyeecheung added a commit that referenced this issue Feb 1, 2019
Instead of using process.config.variables.v8_enable_inspector
to detect whether inspector is enabled in the build.

PR-URL: #25819
Refs: #25343
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this issue Feb 10, 2019
Instead of using process.config.variables.v8_enable_inspector
to detect whether inspector is enabled in the build.

PR-URL: #25819
Refs: #25343
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@silverwind
Copy link
Contributor

This looks like a great way to ease feature detection. I think we should document and require a addition to it for every feature that can not easily be detected (like new options), ideally as part of the PR that adds the feature.

I do believe that it should be frozen thought to be reliable.

@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

I believe we can close this issue. process.features is not going anywhere any time soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants