8000 repl: preload missing `constants` module by addaleax · Pull Request #6494 · nodejs/node · GitHub
  • [go: up one dir, main page]
    Skip to content

    repl: preload missing constants module #6494

    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.

    Sign up for GitHub

    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

    Jump to bottom
    Closed
    addaleax wants to merge 1 commit into from
    Closed

    repl: preload missing constants module #6494

    addaleax wants to merge 1 commit into from

    Conversation

    @addaleax
    Copy link
    Member
    @addaleax addaleax commented Apr 30, 2016
    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.

    @addaleax
    repl: preload missing constants module
    24664c9 
    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 •
    edited
    Loading

    LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/2449/

    @vkurchatkin
    Copy link
    Contributor
    vkurchatkin commented Apr 30, 2016

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

    @addaleax
    Copy link
    Member Author
    addaleax commented Apr 30, 2016

    @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
    Fishrock123 commented May 1, 2016

    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 •
    edited
    Loading

    @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
    bnoordhuis commented May 1, 2016

    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
    Fishrock123 commented May 1, 2016

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

    @jasnell jasnell mentioned this pull request May 1, 2016
    Closed
    1 task
    @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 :)

    @ChALkeR ChALkeR mentioned this pull request May 3, 2016
    Closed
    4 tasks
    @addaleax
    Copy link
    Member Author
    addaleax commented May 13, 2016

    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

    Reviewers

    No reviews

    Assignees

    No one assigned

    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

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    9 participants

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