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

repl: preload missing constants module #6494

Closed

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.
@addaleax addaleax 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
Member Author

@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
Member Author

addaleax commented May 1, 2016

@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

ChALkeR commented May 1, 2016

@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

cjihrig commented May 1, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented May 1, 2016

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?

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

addaleax commented May 2, 2016

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

@addaleax
Copy link
Member Author

Closing in favour of #6534.

@addaleax addaleax closed this May 13, 2016
@sam-github 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants