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

Improve consistency with other JS libraries #19

Merged
merged 7 commits into from
Oct 23, 2023
Merged

Conversation

kearfy
Copy link
Member

@kearfy kearfy commented Oct 23, 2023

This PR:

  • Is replacing ns/db/sc/user/pass with namespace/database/scope/username/password where this was not already the case
  • Ensures the authentication token to be returned when authenticating as a root user
  • Ensures true being returned when authenticating with a token successfully
  • Ensuring that the query/select/create/update/merge/patch/delete methods always return an array (to stay consistent with other JS SDKs)
  • Fixes some of the tests
  • Improved typescript typing

@kearfy kearfy marked this pull request as ready for review October 23, 2023 10:25
Comment on lines -172 to 178
#[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))
}

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?

Copy link
Member Author

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 :)

Copy link
Member Author

@kearfy kearfy Oct 23, 2023

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 {
    // ...
}

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! We could have done that, but I don't think it's a biggie here. Typescript is not as strict as Rust (the type of _true is explicitly true here, but there are no errors or warnings about the else clause)
image

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.

Copy link
Contributor

@RaphaelDarley RaphaelDarley left a 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.

@rushmorem rushmorem changed the title Various fixes Improve consistency with other JS libraries Oct 23, 2023
@rushmorem rushmorem merged commit eeec74c into main Oct 23, 2023
27 checks passed
@kearfy kearfy deleted the micha/various-fixes branch October 23, 2023 13:56
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.

None yet

3 participants