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

Merge Realm JS and Realm Web TypeScript declarations #2980

Closed
18 of 19 tasks
kraenhansen opened this issue Jun 16, 2020 · 4 comments
Closed
18 of 19 tasks

Merge Realm JS and Realm Web TypeScript declarations #2980

kraenhansen opened this issue Jun 16, 2020 · 4 comments

Comments

@kraenhansen
Copy link
Member

kraenhansen commented Jun 16, 2020

Users are experiencing duplicate and diverging types across Realm JS and Realm Web.

Besides merging the type definitions, we also have to update the implementation and any tests relying on this:

  • Rename the identity property on a User to id.
  • Rename the token property on a User to accessToken.
  • Expose the refresh token of a User as refreshToken.
  • Change the allUsers from an instance method to a getter-property.
  • Change the currentUser from an instance method to a getter-property.
  • Make the logOut method on the User return a resolved promise (Promise<void>), instead of returning undefined synchronously.
  • Rename API to Api for the auth related classes (here, here)
  • Rename from UserAPIKeyProviderClient to ApiKeyProvider ApiKeyAuth (loosing User and Client & gaining Auth), in the internal C++ implementation.
  • Rename from EmailPasswordProviderClient to EmailPasswordProvider EmailPasswordAuth (loosing Client & gaining Auth), in the internal C++ implementation.
  • Rename methods of the ApiKeyProvider (loosing the "APIKey" suffix is fine, since its already called on an ApiKeyProvider which has no other methods that handle anything else than api-keys):
    • createAPIKeycreate
    • fetchAPIKeyfetch
    • fetchAPIKeysfetchAll
    • deleteAPIKeydelete
    • enableAPIKeyenable
    • disableAPIKeydisable
  • Rename methods of the EmailPasswordProvider:
    • registerEmailregisterUser
    • resendConfirmationresendConfirmationEmail
  • Add the callResetPasswordFunction method to the EmailPasswordProvider.

NOTE: This should probably be implemented as a PR against the kh/v10-merged-types branch to keep the changes together when merged into v10.

@cmelchior
Copy link
Contributor

Comparing this to the Java types. I saw the following differences:

  • App.logOut() is on User, so User.logOut()
  • We have io.realm.mongodb.auth.ApiKeyAuth. It was based on the conclusions from this discussion where the Provider suffix was deemed to vague: https://docs.google.com/document/d/1Vaciwbl093c252-2F5j3o6u33jLUoQ2A9_8hJ7hMzn0/edit#heading=h.a514whou3ddc. We kept the names from the original Stitch API so createApiKey, deleteApiKey, disableApiKey, enableApiKey, fetchApiKey and fetchAllApiKeys().
  • EmailPassword is called io.realm.mongodb.auth.EmailPasswordAuth. We have registerUser and resendConfirmationEmail

Personal comment:

  • How do you expect the call site to be for the auth provider? I can see loosing APIKey make sense if it is somehow present in the call chain, but not sure how you envision it, so JS would commonly be user.apikeyProvider.create() ?

@kraenhansen
Copy link
Member Author

App.logOut() is on User, so User.logOut()

Yeah - it's already on the User, my suggestion is to add it on the App instance for convenience (since Realm Web already has that).
We could also leave it out (removing it from Realm Web) for simplicity and a unified API across SDKs?

We kept the names from the original Stitch API so createApiKey, deleteApiKey, disableApiKey, enableApiKey, fetchApiKey and fetchAllApiKeys().

Would you be open for a change to simplify this?
There are no methods on the ApiKeyAuth that does not operate on ApiKey's and that suffix is redundant from my perspective (just like Realm#create is not Realm#createObject)

We have registerUser and resendConfirmationEmail

Okay, I'll update Realm Web from resendConfirmationresendConfirmationEmail instead.

How do you expect the call site to be for the auth provider?

await app.currentUser.auth.apiKeys.create( /*...*/ )

@kraenhansen
Copy link
Member Author

We have io.realm.mongodb.auth.ApiKeyAuth. It was based on the conclusions from this discussion where the Provider suffix was deemed too vague.

The way I read the document (and remember the meeting) I thought Java was landing on EmailPasswordAuthProvider and ApiKeyAuthProvider, but I do see the conclusion mentioning "Auth-suffix".

I don't particularly like this name - Auth is equally vague in my opinion, but if that is what we collectively decided, let's go adopt that.

@cmelchior
Copy link
Contributor

Yeah - it's already on the User, my suggestion is to add it on the App instance for convenience (since Realm Web already has that). We could also leave it out (removing it from Realm Web) for simplicity and a unified API across SDKs?

I would probably just keep it on User since it keeps the App "cleaner", but I will leave that up to you.

Would you be open for a change to simplify this?

I'm a bit torn on this because it massively depends on how you envision the usage.

// Java
ApiKeyAuth apiKeys = user.getApiKeyAuth()
apiKeys.create() // Very readable

ApiKeyAuth provider = user.getApiKeyAuth()
provider.create(); // Confusing

// Kotlin
user.apiKeyAuth.create() // Readable
val provider = user.apiKeyAuth
provider.create() // Confusing

I would probably tend to agree with you that your simplications make sense since the type is ApiKeyAuth.create(), and avoiding an API just because we don't think end developers can give good variable names is maybe a bit too pessimistic.

resendConfirmation was a bit too vaque as it wasn't clear exactly how it was sent. In that case resendConfirmationEmail() is much better IMO.

@kneth kneth mentioned this issue Jun 22, 2020
@kraenhansen kraenhansen mentioned this issue Jul 9, 2020
8 tasks
@kraenhansen kraenhansen self-assigned this Jul 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants