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

crypto: set DEFAULT_ENCODING property to non-enumerable #1

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Owner

@aduh95 aduh95 commented Oct 2, 2018

Since it is a deprecated API, a deprecation warning is printed when
loading crypto module from ESM. Making it non enumerable remove the
deprecation warning and make the API non-available to named imports.

Fixes: nodejs#23203

The Credentials API was already removed by nodejs#21153.

It is a breaking change, but IMHO it should be integrated to Node 11 before it's released as it is a minor semver change.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Since it is a deprecated API, a deprecation warning is printed when
loading crypto module from ESM. Making it non enumerable remove the
deprecation warning and make the API non-available to named imports.

Fixes: nodejs#23203
@aduh95
Copy link
Owner Author

aduh95 commented Oct 2, 2018

Aaaannndd I submitted this to the wrong fork

@aduh95 aduh95 closed this Oct 2, 2018
aduh95 pushed a commit that referenced this pull request Nov 7, 2018
Reverting this enables us to provide slower, but longer-lasting
replacements for the deprecated APIs.

Original commit message:

    Put back deleted V8_DEPRECATE_SOON methods

    This partially reverts
    https://chromium-review.googlesource.com/c/v8/v8/+/1177861,
    which deleted many V8_DEPRECATE_SOON methods rather than moving them to
    V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED.

    Note V8_DEPRECATED that were deleted in the same CL stay deleted.

    NOTRY=true
    NOPRESUBMIT=true
    NOTREECHECKS=true

    Bug: v8:7786, v8:8240
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng
    Change-Id: I00330036d957f98dab403465b25e30d8382aac22
    Reviewed-on: https://chromium-review.googlesource.com/1251422
    Commit-Queue: Dan Elphick <delphick@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Reviewed-by: Michael Hablich <hablich@chromium.org>
    Cr-Commit-Position: refs/branch-heads/7.0@{nodejs#49}
    Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1}
    Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{#55424}

Refs: v8/v8@9136dd8
Refs: nodejs#23122

PR-URL: nodejs#23158
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crypto warnings coming up for import crypto from 'crypto'
1 participant