Skip to content

Conversation

@addaleax
Copy link
Member

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

repl, node cli

Description of change

The constants module was previously missing from the list of builtin modules which are available without requiring in the repl and using the --eval command line option. This patch adds it to that list.

The `constants` module was previously missing from the list of builtin modules which are available without requiring in the repl and using the `--eval` command line option. This patch adds it to that list.
@addaleaxaddaleax added the repl Issues and PRs related to the REPL subsystem. label Apr 30, 2016
@mscdex
Copy link
Contributor

mscdex commented Apr 30, 2016

@vkurchatkin
Copy link
Contributor

I think it's missing because it's not documented (=private)

@addaleax
Copy link
MemberAuthor

@vkurchatkin Thought so too, until I saw the PR for adding constants.O_NOATIME. It’s actually explicitly mentioned in the docs for fs.open and for the crypto module.

@Fishrock123
Copy link
Contributor

I don't think it's actually private though; if it were, it would be in process.binding() only I think?

@addaleax
Copy link
MemberAuthor

@Fishrock123 I’d consider the _-prefixed files in lib/ private, too, and they can be require()d like any other core module.

My understanding so far was that, essentially, the modules that are considered public are the ones that are documented, and require('constants') is definitely referenced in the docs.

@jasnell
Copy link
Member

jasnell commented May 1, 2016

@ChALkeR ... are you able to pull together some stats on how many modules are using require('constants')?

@ChALkeR
Copy link
Member

@jasnell Done: https://gist.github.com/ChALkeR/b4d8cca42890718229f82933c721bbe2.

Most of that is error handling, e.g. ENOTSUP/ENOENT/EPIPE.

@bnoordhuis
Copy link
Member

LGTM. require('constants') is in that grey area where it's de facto if not de jure public API. A lot of existing code uses it and you sometimes can't avoid it, e.g., for the SSL_OP_* constants.

@cjihrig
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

Yep.. Thanks @ChALkeR! LGTM.
As a next step we should get documentation for this finally also. @nodejs/documentation

@Fishrock123
Copy link
Contributor

I can't think of a reason why we wouldn't document it?

@addaleaxaddaleax added the wip Issues and PRs that are still a work in progress. label May 2, 2016
@addaleax
Copy link
MemberAuthor

Putting this on hold because #6534 seeks to deprecate the module completely :)

@addaleax
Copy link
MemberAuthor

Closing in favour of #6534.

@sam-githubsam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. labels Dec 1, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.replIssues and PRs related to the REPL subsystem.wipIssues and PRs that are still a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@addaleax@mscdex@vkurchatkin@Fishrock123@jasnell@ChALkeR@bnoordhuis@cjihrig@sam-github