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

build: use node:crypto not crypto when importing #736

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

yharaskrik
Copy link
Contributor

Cloudflare Workers/Pages work with certain native node libraries (https://developers.cloudflare.com/workers/runtime-apis/nodejs) but for them to work correctly they need to be imported with node:crypto instead of crypto (for example).

This PR updates uuid so it imports from node:crypto to resolve this issue.

@ctavan
Copy link
Member

ctavan commented Sep 30, 2023

At a first glance this looks pretty cloudflare worker specific. Would this work in other runtimes as well?

@yharaskrik
Copy link
Contributor Author

At a first glance this looks pretty cloudflare worker specific. Would this work in other runtimes as well?

It does work in other runtimes yes. It's a thing introduced by Node itself.

Here's a snippet of the support:

Supported in Node.js starting:
v16.0.0, v14.18.0 (ESM import and CommonJS require())
v14.13.1, v12.20.0 (only ESM import)
Supported in TypeScript by the latest versions of @types/node.

Looking at it now I am not sure how far back uuid supports so this change may not work within the confines of the node: prefix support. If not then feel free to close this and I'll find a way to rewrite the imports so they work in CloudFlare!

@yharaskrik
Copy link
Contributor Author

Looks like it also works in Deno

https://docs.deno.com/runtime/manual/node/node_specifiers

Any runtime that is compatible with Node should theoretically work with this. But I can look into other runtimes if we deem that this change should be something we want.

The node: prefix was introduced to be more explicit about a module coming from node internal.

@yharaskrik
Copy link
Contributor Author

Looks like CI failed on Node 12, if this lib should work with 12 then unfortunately we will need to close this PR which is fine and I'll just alias crypto to node:crypto.

(Also on my phone it looks like my messages got duplicated, sorry about that!)

@broofa
Copy link
Member

broofa commented Sep 30, 2023

@ctavan: The node: prefix is an official nodejs initiative to reduce the risks of typo-squatting. It's probably a good idea to pull this in.

The only real problem I see here is going to be backward-compat support for earlier versions of node. I just allowed checks here, and it looks like node@12 doesn't support the "node:" prefix.

@yharaskrik Let's keep this open for the time being. Our (not-so?) formal support policy is "node LTS + one previous version", which means we should arguably drop node < 16 from our support matrix, in which case this change would be fine.

@yharaskrik
Copy link
Contributor Author

@ctavan: The node: prefix is an official nodejs initiative to reduce the risks of typo-squatting. It's probably a good idea to pull this in.

The only real problem I see here is going to be backward-compat support for earlier versions of node. I just allowed checks here, and it looks like node@12 doesn't support the "node:" prefix.

@yharaskrik Let's keep this open for the time being. Our (not-so?) formal support policy is "node LTS + one previous version", which means we should arguably drop node < 16 from our support matrix, in which case this change would be fine.

Sounds good to me!

If 18 is LTS shouldn't uuid support 16+? (Unless when you say one previous version you mean 17 and not the previous LTS)

Either way I would think node 12 would be dropped and this would be good to go? (Assuming no one is on node 13 haha).

But either way 16 OR 17 plus would be ok with this change. (According to the compat I found for the node prefix)

@ctavan
Copy link
Member

ctavan commented Sep 30, 2023

The only “problem” is that we need to do a major version bump whenever we drop node version support. But I totally agree that it seems about time to do this again.

@yharaskrik
Copy link
Contributor Author

The only “problem” is that we need to do a major version bump whenever we drop node version support. But I totally agree that it seems about time to do this again.

Well let me know if I can help!

@broofa
Copy link
Member

broofa commented Jun 3, 2024

Just updated our CI matrix to drop support for node < 16, so it should be safe to merge this now.

@broofa broofa merged commit 3fd0f18 into uuidjs:main Jun 3, 2024
4 checks passed
@broofa
Copy link
Member

broofa commented Jun 3, 2024

Thanks for the help, @yharaskrik.

@yharaskrik
Copy link
Contributor Author

Thanks for the help, @yharaskrik.

No problem.

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.

3 participants