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

jasnell · 2016-05-02T22:14:39Z

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:

The constants module was never actually documented. It's referenced a number of times in documentation but never adequately supported.
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.
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 · 2016-05-02T22:16:52Z

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

eljefedelrodeodeljefe · 2016-05-02T22:19:34Z

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

ChALkeR · 2016-05-03T08:32:09Z

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

ChALkeR · 2016-05-03T08:34:03Z

tools/eslint/node_modules/graceful-fs/polyfills.js

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

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 · 2016-05-03T14:13:38Z

@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 · 2016-05-03T15:47:21Z

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 · 2016-05-03T16:05:26Z

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 · 2016-05-03T16:23:44Z

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

ChALkeR · 2016-05-03T16:29:27Z

@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 · 2016-05-03T16:32:04Z

@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 · 2016-05-03T16:33:05Z

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

ChALkeR · 2016-05-03T16:35:24Z

@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 · 2016-05-03T16:40:52Z

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 · 2016-05-03T16:48:18Z

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

ChALkeR · 2016-05-03T16:49:48Z

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

jasnell · 2016-05-03T16:59:05Z

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 · 2016-05-03T16:59:45Z

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

jasnell · 2016-05-17T17:09:57Z

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

jasnell · 2016-05-17T17:29:16Z

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

The require('constants') module is currently undocumented and mashes together unrelated constants. This refactors 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.

jasnell · 2016-05-17T18:04:13Z

CI is green except for unrelated failure.

The require('constants') module is currently undocumented and mashes together unrelated constants. This refactors 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. PR-URL: #6534 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Lindstaedt <[email protected]>

jasnell · 2016-05-17T18:07:21Z

Landed in dcccbfd

zlib constants were previously being added to binding in node_zlib.cc. This moves the zlib constants to node_constants.cc for consistency with the recent constants refactoring: nodejs#6534 Adds require('zlib').constants to expose the constants Docs-only deprecates the constants hung directly off require('zlib') Removes a couple constants from the docs that apparently no longer exist in the code

zlib constants were previously being added to binding in node_zlib.cc. This moves the zlib constants to node_constants.cc for consistency with the recent constants refactoring: #6534 Adds require('zlib').constants to expose the constants Docs-only deprecates the constants hung directly off require('zlib') Removes a couple constants from the docs that apparently no longer exist in the code PR-URL: #7203 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

The require('constants') module is currently undocumented and mashes together unrelated constants. This refactors 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. PR-URL: #6534 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Lindstaedt <[email protected]>

The require('constants') module is currently undocumented and mashes together unrelated constants. This refactors 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. PR-URL: #6534 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Lindstaedt <[email protected]> Conflicts: doc/api/fs.md

Notable changes: * buffer: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) #7157 * build: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) #6994 - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`. * crypto: Root certificates have been updated. (Ben Noordhuis) #7363 * debugger: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) #3316 * npm: Upgraded npm to v3.10.3 (Kat Marchán) #7515 & (Rebecca Turner) #7410 * readline: Added the `prompt` option to the readline constructor. (Evan Lucas) #7125 * repl / vm: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) #6635 * src: - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) #3098 - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) #6534 * stream: Improved `readable.read()` performance by up to 70%. (Brian White) #7077 * timers: `setImmediate()` is now up to 150% faster in some situations. (Andras) #6436 * util: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) #7499 * v8-inspector: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) #6792 - *Note: This feature is experimental, and it could be altered or removed.* - You can try this feature by running Node.js with the `--inspect` flag. Refs: #7441 PR-URL: #7550

### Notable changes * **buffer**: Added `buffer.swap64()` to compliment `swap16()` & `swap32()`. (Zach Bjornson) [#7157](nodejs/node#7157) * **build**: New `configure` options have been added for building Node.js as a shared library. (Stefan Budeanu) [#6994](nodejs/node#6994) - The options are: `--shared`, `--without-v8-platform` & `--without-bundled-v8`. * **crypto**: Root certificates have been updated. (Ben Noordhuis) [#7363](nodejs/node#7363) * **debugger**: The server address is now configurable via `--debug=<address>:<port>`. (Ben Noordhuis) [#3316](nodejs/node#3316) * **npm**: Upgraded npm to v3.10.3 (Kat Marchán) [#7515](nodejs/node#7515) & (Rebecca Turner) [#7410](nodejs/node#7410) * **readline**: Added the `prompt` option to the readline constructor. (Evan Lucas) [#7125](nodejs/node#7125) * **repl / vm**: `sigint`/`ctrl+c` will now break out of infinite loops without stopping the Node.js instance. (Anna Henningsen) [#6635](nodejs/node#6635) * **src**: - Added a `node::FreeEnvironment` public C++ API. (Cheng Zhao) [#3098](nodejs/node#3098) - Refactored `require('constants')`, constants are now available directly from their respective modules. (James M Snell) [#6534](nodejs/node#6534) * **stream**: Improved `readable.read()` performance by up to 70%. (Brian White) [#7077](nodejs/node#7077) * **timers**: `setImmediate()` is now up to 150% faster in some situations. (Andras) [#6436](nodejs/node#6436) * **util**: Added a `breakLength` option to `util.inspect()` to control how objects are formatted across lines. (cjihrig) [#7499](nodejs/node#7499) * **v8-inspector**: Experimental support has been added for debugging Node.js over the inspector protocol. (Ali Ijaz Sheikh) [#6792](nodejs/node#6792) - **Note: This feature is _experimental_, and it could be altered or removed.** - You can try this feature by running Node.js with the `--inspect` flag.

jasnell added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. os Issues and PRs related to the os subsystem. labels May 2, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 2, 2016

jasnell mentioned this pull request May 2, 2016

doc: add initial constants module documentation #6505

Closed

1 task

jasnell added the wip Issues and PRs that are still a work in progress. label May 2, 2016

addaleax mentioned this pull request May 2, 2016

repl: preload missing constants module #6494

Closed

2 tasks

jasnell mentioned this pull request May 3, 2016

process: add O_NOATIME constant #6492

Closed

3 tasks

ChALkeR reviewed May 3, 2016
View reviewed changes

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 3, 2016

jasnell force-pushed the more-logical-constants branch from 4c6c0c9 to e791429 Compare May 3, 2016 16:23

Fishrock123 force-pushed the master branch from 05f4313 to eb12f93 Compare May 3, 2016 16:46

jasnell force-pushed the more-logical-constants branch from 187a525 to d52bc0c Compare May 17, 2016 17:37

jasnell closed this May 17, 2016

lance mentioned this pull request Jun 16, 2016

doc: Clarify fs.access works on directories too. #7113

Closed

2 tasks

Fishrock123 mentioned this pull request Jun 27, 2016

Propose v6.3.0 #7441

Merged

Fishrock123 mentioned this pull request Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

cjean-fr mentioned this pull request Jul 7, 2016

Update nodejs to 6.3.0 ScoopInstaller/Scoop#948

Merged

ChALkeR mentioned this pull request Aug 28, 2016

node: deprecate public access to process.binding #2768

Closed

wdavidw mentioned this pull request Sep 1, 2016

FIX: update sftp after changes in latest nodejs constants mscdex/ssh2-streams#47

Closed

addaleax mentioned this pull request Sep 27, 2016

question about implementation details isaacs/node-graceful-fs#48

Closed

sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. labels Dec 1, 2016

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jasnell commented May 2, 2016 •

edited

Loading

jasnell commented May 2, 2016

eljefedelrodeodeljefe commented May 2, 2016

ChALkeR commented May 3, 2016 •

edited

Loading

ChALkeR May 3, 2016

jasnell May 3, 2016

jasnell commented May 3, 2016

Fishrock123 commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 3, 2016

ChALkeR commented May 3, 2016 •

edited

Loading

jasnell commented May 3, 2016

ChALkeR commented May 3, 2016

ChALkeR commented May 3, 2016

ChALkeR commented May 3, 2016 •

edited

Loading

jasnell commented May 3, 2016

ChALkeR commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 17, 2016 •

edited

Loading

jasnell commented May 17, 2016

jasnell commented May 17, 2016

jasnell commented May 17, 2016

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

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

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

Conversation

jasnell commented May 2, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell commented May 2, 2016

eljefedelrodeodeljefe commented May 2, 2016

ChALkeR commented May 3, 2016 • edited Loading

ChALkeR May 3, 2016

Choose a reason for hiding this comment

jasnell May 3, 2016

Choose a reason for hiding this comment

jasnell commented May 3, 2016

Fishrock123 commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 3, 2016

ChALkeR commented May 3, 2016 • edited Loading

jasnell commented May 3, 2016

ChALkeR commented May 3, 2016

ChALkeR commented May 3, 2016

ChALkeR commented May 3, 2016 • edited Loading

jasnell commented May 3, 2016

ChALkeR commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 3, 2016

jasnell commented May 17, 2016 • edited Loading

jasnell commented May 17, 2016

jasnell commented May 17, 2016

jasnell commented May 17, 2016

jasnell commented May 2, 2016 •

edited

Loading

ChALkeR commented May 3, 2016 •

edited

Loading

ChALkeR commented May 3, 2016 •

edited

Loading

ChALkeR commented May 3, 2016 •

edited

Loading

jasnell commented May 17, 2016 •

edited

Loading