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

node and bun compatibility #14

Merged
merged 9 commits into from
Nov 3, 2023
Merged

node and bun compatibility #14

merged 9 commits into from
Nov 3, 2023

Conversation

FiloSottile
Copy link
Owner

Reopening #13 which I merged by mistake.

FiloSottile and others added 6 commits November 3, 2023 11:01
Fixes Node module resolution.

Partially based on #11 by @lgarron.

Fixes #9
Co-authored-by: Lucas Garron <code@garron.net>
isolatedModules is recommended by esbuild at
https://esbuild.github.io/content-types/#isolated-modules

module is required by moduleResolution.

Co-authored-by: Lucas Garron <code@garron.net>
Co-authored-by: Lucas Garron <code@garron.net>
Co-authored-by: Lucas Garron <code@garron.net>
@FiloSottile
Copy link
Owner Author

So huh, I guess the const assignment I am doing in non-test code is also not ok, but happens to work because non-crypto functions don't need to await ready. I hate this.

Apparently you can't even take the function pointer of crypto functions
before sodium.ready.

We continue to take the pointer for non-crypto functions since we do it
in format.ts.
Fixes #7
Closes #8

Co-authored-by: Lucas Garron <code@garron.net>
@lgarron
Copy link
Contributor

lgarron commented Nov 3, 2023

So huh, I guess the const assignment I am doing in non-test code is also not ok, but happens to work because non-crypto functions don't need to await ready.

If you do the const assignment after libsodium.ready I think you should be able to destructure everything the same way you do imports. But what you have now also works (assuming `libsodium-wrappers-sumo' doesn't change instantiation semantics for the non-crypto methods).

// before
import { crypto_hash_sha256, from_hex, to_hex } from "libsodium-wrappers-sumo";
// after
import sodium from "libsodium-wrappers-sumo"

await sodium.ready
const { crypto_hash_sha256, from_hex, to_hex } = sodium; // now we can get crypto as well as non-crypto exports

Or you could just YOLO and make all your APIs async (which would have the benefit of allowing automatic offloading to a web worker for key derivation in the future if you want). 😛

@FiloSottile
Copy link
Owner Author

If you do the const assignment after libsodium.ready I think you should be able to destructure everything the same way you do imports. But what you have now also works (assuming `libsodium-wrappers-sumo' doesn't change instantiation semantics for the non-crypto methods).

I filed jedisct1/libsodium.js#327 to hopefully get even the crypto functions to be safe to assign before ready. If it doesn't get resolved that way, maybe I'll switch everything to be safe.

In the test files it's a matter of switching the order, but in format.ts will require just not destructuring, because we don't call ready ourselves.

Or you could just YOLO and make all your APIs async (which would have the benefit of allowing automatic offloading to a web worker for key derivation in the future if you want). 😛

I had that in a previous iteration and kinda hated that it gave the impression the whole operation would be async, and then we'd block for scrypt after maybe awaiting ready (the first time).

@FiloSottile
Copy link
Owner Author

Now with Puppeteer tests for esbuild! Thank you for all the pointers @lgarron. Let me know if there's anything missing from #11.

@FiloSottile FiloSottile merged commit f6655a6 into main Nov 3, 2023
4 checks passed
@FiloSottile FiloSottile deleted the filippo/compat branch November 3, 2023 18:35
@FiloSottile FiloSottile mentioned this pull request Nov 3, 2023
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.

2 participants