crypto: use globalThis.crypto over require('crypto').webcrypto by panva · Pull Request #45817 · nodejs/node · GitHub

Updates tests and some remaining documentation to use globalThis.crypto instead of require('crypto').webcrypto.

@aduh95 it would be great if we could make this into a lint rule but unfortunately the existing options I tried to do this with fall short:

no-restricted-modules cannot restrict an exported property from the crypto module.
no-restricted-properties does not catch use of const { webcrypto } = require('crypto').

Review requested:

@nodejs/crypto

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

I disagree with the changes in the test, we want to keep the test as is to ensure there's no regressions. The doc-changes LGTM.

I disagree with the changes in the test, we want to keep the test as is to ensure there's no regressions. The doc-changes LGTM.

What test change do you disagree with?

What test change do you disagree with?

Changing the tests to test globalThis.crypto instead of require('node:crypto').webcrypto.

What test change do you disagree with?

Changing the tests to test globalThis.crypto instead of require('node:crypto').webcrypto.

We have other existing tests that ensure the two are equal. So I don't see why these changes would be a cause for concern.

We have other existing tests that ensure the two are equal.

That seems like another reason not to make this change. Unless we decide to deprecate require('node:crypto').webcrypto, there's no reason to remove it from our tests IMO.

I'm unforunately not following your argumentation @aduh95.

I don't see the need for having to have a deprecation on it to recognize the global is the likely way of accessing a Web API, it is the way we already have in most doc examples, and it should therefore be the one tested.

I guess we would have to agree to disagree, and wait to see if someone else wants to chime in. If someone else thinks we should merge it, I'm ready to dismiss my review.

Count me as -.5 on this because:

We usually don't accept PRs that are doing "stylistic changes" without a lint rule, and arguably accessing the API one way or another is a question of personal preference and qualify as a "stylistic change" IMO.
Even if there were a lint rule added with this PR, I would still be against it (but wouldn't block it), IMO both ways are "valid", and there's little value in enforcing one over the other.

and it should therefore be the one tested.

I disagree, the fact that one API is used less often than another doesn't mean it deserves less test coverage (but anyway it's a bit off-topic, since both APIs are equivalent in this case).

I agree with @aduh95 regarding the usage of require('node:crypto').webcrypto instead of globalThis.crypto internally.

I'm in favor of blocking this because, if we are going to use globalThis.crypto instead of require('node:crypto').webcrypto we need a lint rule to protect this change in the upcoming pull requests.

if we are going to use globalThis.crypto instead of require('node:crypto').webcrypto we need a lint rule to protect this change in the upcoming pull requests.

And I've requested assistance with such in the very description of this PR myself.

And I've requested assistance with such in the very description of this PR myself.

I totally missed that part, my bad!

I see. I found the corresponding bug report for this: eslint/eslint#16412

I've added a working, albeit rudimentary, lint rule.

lib/.eslintrc.yaml

Not while it's defined using no-restricted-syntax. When no-restricted-properties is fixed and used, sure.

lib/ should never use the global (which is user-mutable) but instead require the internal module. There's already a lint rule for that:

node/lib/.eslintrc.yaml

Lines 150 to 155 in 587367d

    
           - name: crypto 
        
             message: Use `const { crypto } = require('internal/crypto/webcrypto');` instead of the global. 
        
           - name: Crypto 
        
             message: Use `const { Crypto } = require('internal/crypto/webcrypto');` instead of the global. 
        
           - name: CryptoKey 
        
             message: Use `const { CryptoKey } = require('internal/crypto/webcrypto');` instead of the global.

Still -1 on this, I don't think this lint rule is a great addition, but won't block it.

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

Landed in 7ad069c

PR-URL: #45817 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>

panva added dont-land-on-v16.x labels Dec 11, 2022

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Dec 11, 2022

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2022

panva force-pushed the crypto-use-global-webcrypto branch from 9c778e5 to bdc2204 Compare December 11, 2022 13:24

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 11, 2022

aduh95 previously requested changes Dec 11, 2022

View reviewed changes

anonrig requested changes Dec 11, 2022

View reviewed changes

panva requested review from jasnell and tniessen December 11, 2022 17:56

panva mentioned this pull request Dec 11, 2022

doc: remove last example use of require('crypto').webcrypto #45819

Closed

anonrig approved these changes Dec 11, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

panva force-pushed the crypto-use-global-webcrypto branch from bdc2204 to 3e0b149 Compare December 11, 2022 19:54

This comment was marked as resolved.

Sign in to view

anonrig approved these changes Dec 11, 2022

View reviewed changes

anonrig reviewed Dec 11, 2022

View reviewed changes

Comment thread .eslintrc.js Outdated

crypto: use globalThis.crypto over require('crypto').webcrypto

cac299a

panva force-pushed the crypto-use-global-webcrypto branch from 3e0b149 to cac299a Compare December 11, 2022 20:02

This comment was marked as resolved.

Sign in to view

anonrig approved these changes Dec 11, 2022

View reviewed changes

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2022

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Dec 13, 2022

CI Reliability 2022-12-13 nodejs/reliability#456

Open

13 tasks

panva removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 13, 2022

This was referenced Dec 14, 2022

CI Reliability 2022-12-14 nodejs/reliability#457

Open

CI Reliability 2022-12-15 nodejs/reliability#458

Open

CI Reliability 2022-12-16 nodejs/reliability#459

Open

jasnell approved these changes Dec 16, 2022

View reviewed changes

panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 16, 2022

nodejs-github-bot merged commit 7ad069c into nodejs:main Dec 16, 2022

panva deleted the crypto-use-global-webcrypto branch December 16, 2022 19:59

targos pushed a commit that referenced this pull request Jan 1, 2023

crypto: use globalThis.crypto over require('crypto').webcrypto

c9747f1

PR-URL: #45817 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>

RafaelGSS mentioned this pull request Jan 2, 2023

v19.4.0 proposal #46061

Merged

crypto: use globalThis.crypto over require('crypto').webcrypto by panva · Pull Request #45817 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: use globalThis.crypto over require('crypto').webcrypto - #45817

crypto: use globalThis.crypto over require('crypto').webcrypto#45817nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom panva:crypto-use-global-webcryptopanva/node:crypto-use-global-webcryptoCopy head branch name to clipboard

Conversation

panva commented Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 11, 2022

Uh oh!

nodejs-github-bot commented Dec 11, 2022

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

panva commented Dec 11, 2022

Uh oh!

aduh95 commented Dec 11, 2022

Uh oh!

panva commented Dec 11, 2022

Uh oh!

aduh95 commented Dec 11, 2022

Uh oh!

panva commented Dec 11, 2022

Uh oh!

aduh95 commented Dec 11, 2022

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

panva commented Dec 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig commented Dec 11, 2022

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

anonrig commented Dec 11, 2022

Uh oh!

panva commented Dec 11, 2022

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

aduh95 commented Dec 11, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Dec 12, 2022

Uh oh!

nodejs-github-bot commented Dec 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

crypto: use globalThis.crypto over require('crypto').webcrypto#45817
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
panva:crypto-use-global-webcrypto

panva commented Dec 11, 2022 •
edited
Loading

panva commented Dec 11, 2022 •
edited
Loading