Skip to content

Conversation

valadaptive
Copy link
Contributor

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Resolves #134. Instead of taking an external dependency on randombytes, which accesses global (doesn't work in the browser) and depends on a Buffer polyfill, we check first for crypto.getRandomValues support and only import Node's crypto module if it's not found.

If you're OK with dropping support for Node <19, the crypto global is available in newer versions of Node (it technically didn't stop being "experimental" until Node 23, but I don't get any annoying ExperimentalWarning messages when trying to access it in older versions).

An alternative option would be to add separate browser and Node entrypoints; this would avoid the dynamic require that might(?) cause trouble for some bundlers. I have that code in a separate branch if you'd prefer it.

@okuryu
Copy link
Collaborator

okuryu commented Oct 1, 2025

Since I was planning to bump the major version anyway when removing randombytes from the dependencies, I don’t think it’s a problem to take this opportunity to support only Node.js 20 and above. Therefore, I don’t think a fallback for require('crypto') is strictly necessary.

By the way, is my understanding correct that this patch also resolves the browser support issue?

@valadaptive
Copy link
Contributor Author

I've updated the PR to use the crypto global only, and added "engines": {"node": ">=20.0.0"} to the package.json.

By the way, is my understanding correct that this patch also resolves the browser support issue?

I haven't tested it in the browser, but it should. We're not accessing global like the randombytes package was.

@okuryu
Copy link
Collaborator

okuryu commented Oct 1, 2025

Got it. Let’s give it a try. Could you remove Node.js v18 from the GitHub Actions config?

@valadaptive
Copy link
Contributor Author

Could you remove Node.js v18 from the GitHub Actions config?

Whoops; just did that. Node 24 is out, should I add that to the test matrix?

@okuryu
Copy link
Collaborator

okuryu commented Oct 2, 2025

Yeah, please go ahead and add 24 as well 🙏

@valadaptive
Copy link
Contributor Author

Added it; still needs approval to run but everything passes locally (as expected)

@okuryu
Copy link
Collaborator

okuryu commented Oct 3, 2025

Could you try running npm install again? It will likely update the package-lock.json file.

@valadaptive
Copy link
Contributor Author

Huh, I didn't know the engines field was part of the lockfile.

As far as I can tell, this... never did anything? My guess is that
`crypto.randomBytes = oldRandom;`
was supposed to come *after* we require()'d `serialize`, but it just
didn't, so it ended up doing nothing.
@okuryu okuryu merged commit e7709f2 into yahoo:main Oct 4, 2025
3 checks passed
@okuryu
Copy link
Collaborator

okuryu commented Oct 4, 2025

Thanks! We’ve released v7.0.0, so please give it a try. Let us know if you run into any issues.

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.

Can't used in browser?
2 participants