-
Notifications
You must be signed in to change notification settings - Fork 24
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
API Improvment #9
Comments
Good work! I would actually go ahead and remove |
I'm happy help :) Will open a PR in the next few days. I have a few questions first though. Is this finialised? const upash = new UPASH({
argon2: require('@phc/argon2'),
pbkdf2: require('@phc/pbkdf2')
}, {default: 'argon2'}); Is there ever going to be any other options passed in the second object? If not I would favour this approach: const upash = new UPASH({
argon2: require('@phc/argon2'),
pbkdf2: require('@phc/pbkdf2')
}, 'argon2'); Also what do we want the behaviour to be if no default is specified? |
Thank you @gavinhenderson !!!
It should definitely throw an error.
Not sure, I'm open to suggestions. I was also thinking at something like that: const upash = new UPASH({
argon2: {algoritmh: require('@phc/argon2'), default: true}
pbkdf2: {algoritmh: require('@phc/pbkdf2'), options: {someParamToOverride: 'somevalue'}}
}); But it seems over complicated. |
I completely agree, people (including me) are turned off to a package at the first sign of a weird API so we want to avoid that as much as possible. Not sure I like the idea of specifying the default in the algorithm object as this could to lead to people entering multiple defaults which we could handle but I think its unnecessary. I would favour the one you originally suggested: const upash = new UPASH({
argon2: require('@phc/argon2'),
pbkdf2: require('@phc/pbkdf2')
}, {default: 'argon2'}); This allowing for easy expansion of the options object which I like. Maybe would could make it more clear in the usage guide (when we get to that) and do something like: const options = { default: 'argon2' }
const upash = new UPASH({
argon2: require('@phc/argon2'),
pbkdf2: require('@phc/pbkdf2')
}, options); |
@gavinhenderson Yeah I totally agree, I was just throwing ideas. |
Description
Expose a constructor to allow the users to have multiple instance of upash instead of having it as a singleton.
Examples
Notes
Probably it makes sense to remove
install
anduninstall
methods and add agetDefault
method.This would lead to a breaking change.
cc @mcollina
The text was updated successfully, but these errors were encountered: