src: refactor constants, "deprecate" require('constants')

[jasnell]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

constants, os, fs, crypto

Description of change

The require('constants') module is currently undocumented and mashes together unrelated constants. This soft deprecates the require('constants') in favor of distinct os.constants, fs.constants, and crypto.constants that are specific to the modules for which they are relevant. The next step is to document those within the specific modules.

The justification for this is straightforward:

1. The constants module was never actually documented. It's referenced a number of times in documentation but never adequately supported.
2. The single constants object mashes up a number of unrelated constants, which can cause issues with sloppy code. For instance, a bug in spawnSync (https://github.com/nodejs/node/blob/master/lib/child_process.js#L383-L391) essentially allows any defined constant to be used as a signal. In another instance, the code is implementing hacks (https://github.com/nodejs/node/blob/master/lib/internal/process.js#L171-L174) to limit look ups to just signals.
3. Constants should be exported by the modules in which they are actually relevant, and they should be logically grouped so that they make sense.

On the downside, there's a slight bit more overhead in creating the process.binding('constants').

/cc @addaleax @nodejs/documentation @nodejs/ctc @nodejs/crypto

[jasnell]

Marking this in-progress because additional work is required to fill out the documentation for constants

[eljefedelrodeodeljefe]

Good idea. nit: Can you indent the table HTML in the docs?

[ChALkeR]

@jasnell What is the reason behind grouping them to .constants? Why not just module.CONSTANT?

[ChALkeR]

I don't think this file should be changed in this commit.

[jasnell]

Whoops! Picked up a stray!
On May 3, 2016 1:34 AM, "Сковорода Никита Андреевич" <
[email protected]> wrote:

In tools/eslint/node_modules/graceful-fs/polyfills.js
#6534 (comment):

@@ -1,5 +1,4 @@
var fs = require('./fs.js')
-var constants = require('constants')

I don't think this file should be changed in this commit.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6534/files/4c6c0c9b0898ef360b2ce648cf7cf41d3d22d560#r61851725

[jasnell]

@ChALkeR ... I went with the {module}.constants approach largely because of how the constants are defined (in a process.binding) and because it helps isolate them. For instance, look at the os.constants.signals, we have code that validates signals currently by just checking to see if the signal string appears as a property on the constants object. The current code is pretty much a hack but this change isolates the signal constants on their own object so that the check can be done without the additional hacks. Also, I'd prefer not to muddy up the main module API with too many constants that may not be used extensively.

I have no problem with exposing the constants directly off the module itself but this way just seems cleaner and more natural to me.

[Fishrock123]

Marking as major because the deprecation and pretty radical change. I've yet to take a detailed look, but I think I'm in favor of this approach.

[jasnell]

FWIW, given that require('constants') was never actually documented and given the fact that there is no hard deprecation, and require('constants') continues to work as it did before, it would likely be ok for this to be a minor. In fact, we could simply choose not say anything about deprecating require('constants').

[jasnell]

@ChALkeR .. updated to remove the errant tool dependency change.

[ChALkeR]

@Fishrock123 There is no actual deprecation here, this looks like a semver-minor to me.

As there are no docs, the next step would be to print a warning on require('constants'), that one would be a semver-major.

[jasnell]

@ChALkeR ... can you do a quick scan of the modules to see how many hits you get for require('constants'). In the initial version of this I had a hard deprecation for require('constants') and just running lint caused a bunch of warnings because of dependencies inside eslint. I'm thinking that if we did ever hard deprecate require('constants') we'd want to wait a while and give some heads up first.

[ChALkeR]

@jasnell That was already done in #6494 (comment).

[ChALkeR]

@jasnell The hard deprecation clearly should not happen in the next major (or even in 8.0), because that will require all those modules (like npm) to utilize feature detection.

[ChALkeR]

I would prefer graceful-fs to include feature detection there asap, though — just too many modules are affected through it.
It has about 50% of all downloads with require('constants').

It should be as trivial as var constants = fs.constants || require('constants').

[jasnell]

yeah, that's what I was afraid of. Ok, let's keep it as a soft deprecation for a long time then :-)

[ChALkeR]

@jasnell I wouldn't call this a soft deprecation — there are no docs for constants to deprecate them =).

[jasnell]

Yes, but there are at least a couple of existing mentions of require('constants') tho in a couple of places even if the module itself is not explicitly documented. I dunno... I'm perfectly happy with this as a semver-minor because the chance of it breaking anything is pretty small.

[jasnell]

btw, CI: https://ci.nodejs.org/job/node-test-pull-request/2483/

[jasnell]

hmm.. getting some issues with the new N_NOATIME constant on linux... investigating now.

[jasnell]

https://ci.nodejs.org/job/node-test-pull-request/2678/

[jasnell]

CI is green except for unrelated failure.

[jasnell]

Landed in dcccbfd