-
Notifications
You must be signed in to change notification settings - Fork 10
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
Improve consistency with other JS libraries #19
Conversation
#[napi] | ||
pub async fn authenticate(&self, token: String) -> Result<()> { | ||
#[napi(ts_return_type="Promise<boolean>")] | ||
pub async fn authenticate(&self, token: String) -> Result<Value> { | ||
self.db.authenticate(token).await.map_err(err_map)?; | ||
Ok(()) | ||
Ok(Value::Bool(true)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Wasm library returns a unit, like the previous implementation. By returning a boolean here, I presume when the function runs successfully it will always return true
but never false
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will return true
or it will throw an error :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with returning void
, well... undefined
, is that you cannot nicely check if the authentication succeeded. Now you can:
if (await db.authenticate('...')) {
// ...
}
Or you can catch the error and change the response based on that
const success = await db.authenticate('...')
.catch((error) => false);
if (success) {
// ...
} else {
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In Rust we typically avoid using booleans in cases like that, preferring a unit instead, because a boolean gives an impression that you will need to account for a false
response too. Which is never the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not about compiler errors or warnings. In Rust you won't get those either when using an if
statement. You would have to use match
for that. The issue was the impression it gives fellow developers when looking at the return type or when looking code examples. They may feel that they need to write an else branch to be extra careful but that else branch is unnecessary and never triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, particularly improved tests, and ts typing.
This PR:
true
being returned when authenticating with a token successfully