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

chore: support deno 2.0 by replacing __dirname -> import.meta.dirname. #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mash8
Copy link

@mash8 mash8 commented Nov 4, 2024

For this issue #4

@mash8
Copy link
Author

mash8 commented Nov 5, 2024

Tests keep failing because of Uncaught TypeError: h is not a function trying to figure it out.

On further investigation it's crashing while trying to print why it crashed...

Real error is

"Error: Both wasm and asm failed to loadTypeError: Cannot read properties of undefined (reading 'rc')\n    at file:///C:/Repos/sodium-fork/dist/browsers/sodium.js:49916:35"

@eliassjogreen
Copy link
Member

Strange, and you get the same error locally when running the tests? Seems like the built code is somehow wrong.

@mash8
Copy link
Author

mash8 commented Nov 5, 2024

Getting it locally as well yeah. I'm gonna give it a shot on a Linux machine see if it makes a difference.

Also another idea.. do you know where the build code originally came from? Maybe there's an updated version?

@mash8
Copy link
Author

mash8 commented Nov 5, 2024

Tried running the tests on deno 1.x and it worked. I guess the js build files don't work in deno 2.x. Need to figure out how to update them.

There wouldn't happen to be a un-uglified version of the sodium.js build file somewhere?

@mash8
Copy link
Author

mash8 commented Nov 6, 2024

Looks like the files came from here originally https://github.com/jedisct1/libsodium.js. I'll try and update them and see what happens.

@eliassjogreen
Copy link
Member

Have you tried using libsodiumjs (or even the web standard crypto if that fits your needs) directly? It may be easier to depend on it than this, honeslty ill-maintained wrapper module.

@mash8
Copy link
Author

mash8 commented Nov 12, 2024

I hadn't heard of libsodiumjs until I started looking into this. It would be easier to just use that... but I it'd be nice if this was working for Deno users. So they can use sodium without needing to think.

I don't mind continuing to work on this if no one else is gonna do it. It's an excuse to learn something on my end.

@eliassjogreen
Copy link
Member

I hadn't heard of libsodiumjs until I started looking into this. It would be easier to just use that... but I it'd be nice if this was working for Deno users. So they can use sodium without needing to think.

Well, if you are doing anything requiring libsodium and crypto, I would hope the user thinks long and hard about the problem theyre trying to solve if it has anything to do with authentication, signing, etc. But I get the need for basic access to cryptographic functions when for example interacting with an API.

I don't mind continuing to work on this if no one else is gonna do it. It's an excuse to learn something on my end.

Go for it! I would appreciate it!

- support deno 2.0 by replacing __dirname -> import.meta.dirname.
- Replaced https://github.com/denolib/setup-deno?tab=readme-ov-file github action with https://github.com/denoland/setup-deno since the former is depreciated.
- Removed a bunch of redundant files.
- Added tests for sumo sodium
- crypto_pwhash & crypto_pwhash_str no longer exited in the non sumo version so moved the typing (there may be more method that are missing).
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the sub repo was redundant so I removed it. Along with a bunch of other files.

@@ -657,31 +657,6 @@ export interface Sodium {
outputFormat: StringOutputFormat,
): StringCryptoKX;

crypto_pwhash(
Copy link
Author

@mash8 mash8 Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these two method no longer existed on the non sumo sodium for some reason. I only caught this because of the tests. Not sure what else could of been impacted.

I moved these methods to the SumoAddons type since it exists there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got the new version of the file from the sodiumjs repo and patched it up to work with deno.
Also it's no longer minified. Didn't see a reason that it needed to be. Was making it hard to work with...

@mash8
Copy link
Author

mash8 commented Nov 15, 2024

Btw @eliassjogreen I noticed you're also apart of the https://github.com/denosaurs/opus repo. I created a fix for that repo here denosaurs/opus#3. It's a lot simpler than this...

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