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

Readd proper async/await keywords to examples/CLI, add default case in cli.ts #10

Closed
wants to merge 1 commit into from

Conversation

prettykool
Copy link
Contributor

Regarding the async thing - As you have mentioned in commit 2e7997e11dd652f1dcfed0d84564ef6c76a7d2a7, you believe that the WASM build had made the need to use async/await redundant. I want you to know, from my experience using it, that's not true.

Let's just use the example you gave out in the README, and break it down piece by piece...

pk ~ > deno
Deno 1.24.2
exit using ctrl+d or close()
> import { hash, verify } from "https://deno.land/x/scrypt/mod.ts";

undefined
> const hashResult = hash("password");

undefined
> hashResult
Promise {
"c2NyeXB0AA4AAAAIAAAAAbb0SyORv+D5hZnWGFdooht5tuoqlM4JTdIYKQPcQTlrUEYByCq20d2+2an6H3DV8+Embr96XNQULJ1X..."
}

Notice that it returns a Promise? That's a pretty big deal, because the lack of an await statement doesn't cause the value to be registered as properly fulfilled. Indeed, if we try to move onto the next piece...

> const verifyResult = verify("password", hashResult);

undefined
> verifyResult
Promise {
  <rejected> TypeError: formattedHash.substring is not a function
    at detectFormat (https://deno.land/x/[email protected]/lib/helpers.ts:119:25)
    at verify (https://deno.land/x/[email protected]/mod.ts:77:28)
    at <anonymous>:2:22
}

It's marked as Rejected. The code given is literally unusable.

Now, if we add the proper await keywords...

> const hashResult = await hash("password");

undefined
> hashResult
"c2NyeXB0AA4AAAAIAAAAAcn3f65D+SUQK0l83StELYsu0viUlDYJH5lH7jwYvn3d0jRCN9w7w0tvxaqCyk1YL6u6VIU5aemLUvh3ysZK0hF/AEiOwfk4RGBb4yEy2agN"
> const verifyResult = await verify("password", hashResult);

undefined
> verifyResult
true

It works as intended.

This issue has been addressed in both the already-described README, and the CLI you wrote as a demonstration of the library.

Oh, and while we're on the subject of the library - A default case was added so that any users who didn't provide a hash or verify flag would receive the following:

usage: hash <password>, verify <password>, <hash>

I just thought that would be nice.

@oplik0
Copy link
Member

oplik0 commented Aug 10, 2022

I think you have a cached older version (probably v2.2.0)? Can you try with

import { hash, verify } from "https://deno.land/x/[email protected]/mod.ts";

Because, well, it works for me:
correctly working example
But specifying v2.2.0 allows me to replicate the issue:
doesn't work on v2.2.0

I'll change the README to use the versioned paths (I should've done that from the start tbh), but I think not suggesting incorrectly that it's still asynchronous would be preferred.

@prettykool
Copy link
Contributor Author

prettykool commented Aug 10, 2022

I checked it using the code from the mainline, and you were right -

Deno 1.24.2
exit using ctrl+d or close()
> Deno.cwd()
"/home/pk/Documents/Builds/scrypt"
> import { hash, verify } from './mod.ts';
undefined
> hash('e');
"c2NyeXB0AA4AAAAIAAAAAYCg6WY/NksDaQofLx9OlAE5rxAO6C9YsBe0Bizu3lRCAm3Gql4i5lP2tO/nkJljl1oWGnOuM8xeQDRJuDZbXbF5ygoF5z2K5bH/WJ8N0lY/"
> verify('e', hash('e'));
true

I don't recall ever downloading this package beforehand - Maybe it was a dependency of another thing I used at some point(?) Beats me. Either way, it ended up in my cache, and ended up being implicitly used in place of the more recent version.

I'll change the README to use the versioned paths (I should've done that from the start tbh), but I think not suggesting incorrectly that it's still asynchronous would be preferred.

Yeah, that's a good move - It'd prevent some confusion for cases like mine, for the former, and for the latter, seeing as this is distinctly not asynchronous (anymore), it's redundant to include.

I'll close this now, seeing as the problem was more a failing on my part than the module itself.

(Quick side-question: Any thoughts on the default case?)

@prettykool prettykool closed this Aug 10, 2022
@oplik0
Copy link
Member

oplik0 commented Aug 10, 2022

I added the default case and included you as the co author of that commit. Thanks!

I'm thinking about re-writing the CLI to have more features (non-default parameters for one), better docs, etc. - and use a proper argument parser instead of a switch/case. But for now this default case is a definitely improvement over no help included at all.

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