doc,crypto: import from 'node:crypto' instead of import('node:crypto')

[marco-ippolito]

Affected URL(s)

https://nodejs.org/api/crypto.html#crypto

Description of the problem

in the esm example of the first paragraph there is this import:
const { createHmac } = await import('node:crypto');
It seems weird, shouldn't be import { createHmac } from 'node:crypto': ?
If so I can open a PR to fix it

[panva]

This is intentional. Because Node.js can be built without crypto support and only the function-like import() can be caught.

https://nodejs.org/api/crypto.html#determining-if-crypto-support-is-unavailable

[marco-ippolito]

Is it the standard to be built without crypto support? Shouldnt the example be with the most common use case?

[panva]

It is what got consensus in #37594

[marco-ippolito]

It is what got consensus in #37594

All right I'm gonna close this as it seems not to be a bug

[ShogunPanda]

I wonder if now ESM import throws and therefore we can remove this special use case.

@nodejs/tsc Tagging you because probably you have more insight on it.

[aduh95]

Node.js can still be built without crypto support. If you want to right your code in a way that takes this into account, you can't use static imports and need to try/catch a dynamic import. If you only want to support Node.js built with crypto, you can use static imports.

I wonder if now ESM import throws and therefore we can remove this special use case.

Importing node:crypto from ESM on a build without support for it has always thrown AFAIK, not sure what you mean @ShogunPanda.

[ShogunPanda]

@aduh95 Nothing, I misunderstood what was the reason in the issue above.
Now that ESM has settled for a while, do you think we should change to be consistent with the rest of the docs?

[aduh95]

The use of dynamic imports in crypto.md had nothing to do with ESM having settled or not, the reason for doing it (some Node.js builds do not ship with node:crypto) is still valid – or rather, is as valid as it was at the time. I personally don't have an opinion one way or another, and certainly don't oppose re-evaluating our position on this, I'm happy to defer to folks who have more thoughts on the topic.

[ShogunPanda]

My opinion here is that we should write example for the majority of users, and eventually a specific example right after for people that don't have crypto enable.

@panva Any thought on this?

[panva]

Sure, that was my position in the original PR too but at that point this is what we agreed to land.

#37594 (review)

[GeoffreyBooth]

It seems weird that the example uses await import() but it isn’t within a try block:

const { createHmac } = await import('node:crypto');

I would think it should look something like this:

let createHmac;
try {
  ({ createHmac } = await import('node:crypto'));
} catch {
  console.error('This Node.js was built without crypto support');
}

[panva]

Neither are the CJS examples but the point is that the syntax used is try-catcheable.

[marco-ippolito]

Reopening at it seems to have created a discussion.
Normally the user would not use a custom build without crypto, so in my opinion the documentation should reflect that.
We should obviously explain that in the case crypto module is not included in the build it should be handled and provide and example.
If you agree I can create a PR to change this.

[panva]

cc @tniessen @jasnell

[jimmywarting]

Not so particular fan of using this pattern:

let xyz
try {
  xyz = await promise
} catch (err) {}

The problem then is that it ain't so type/ide friendly without having to add extra annotation. + it is now mutable instead of being a const.
ppl overlook the use of catch now that we got async/await

i would write it more like:

const nodeCrypto = await import('node:crypto').catch(() => {})

const hash = nodeCrypto?.createHmac('sha256', secret)
  .update('I love cupcakes')
  .digest('hex')

if (hash) { ... }
if (nodeCrypto) { ... }

or actually use good old .then(...) (not everything must be written with async/await) sometimes i try to break out of promise chain hell when i can do so.