wasp icon indicating copy to clipboard operation
wasp copied to clipboard

Somehow give more context to some of the symbols exported from our SDK

Open Martinsos opened this issue 1 year ago • 3 comments

This mostly goes for wasp/auth/client and wasp/auth/server -> there are a lot of symbols being exports from these ones, and some of them are in my opinion lacking context to make it easy to understand what they are about.

So wasp/server/auth exports all of the following identifiers:

defineUserSignupFields

ensurePasswordIsPresent
ensureValidPassword

ensureValidEmail

ensureValidUsername

createProviderId
sanitizeAndSerializeProviderData
updateAuthIdentityProviderData
deserializeAndSanitizeProviderData
findAuthIdentity
createUser

GetVerificationEmailContentFn
GetPasswordResetEmailContentFn
createEmailVerificationLink
sendEmailVerificationEmail

Some of this stuff is valid only for email&pass, some of it only for username&pass -> I think if I was reading code using these functions, I would love to have that be relatively clear from the name or at least from the file I am reading (so name + import), instead of having to go check the docs, be it manually or via IDE.

Maybe I am overthinking it.

Let's take a look at wasp/client/auth:

useAuth
logout

LoginForm
SignupForm

ForgotPasswordForm
VerifyEmailForm
ResetPasswordForm

CustomizationOptions

GoogleSignInButton

GithubSignInButton

FormError
FormInput
FormItemGroup
FormLabel

login (email)
signup (email)
requestPasswordReset
resetPassword
verifyEmail

login (username)
signup (username)

googleSignInUrl
gitHubSignInUrl

CustomizationOptions sounds very out of context, we don't know what it is here to customize. FormError, FormInput, ... -> those also sound like they are missing some context, what are these about?

There is this situation with login and signup sounding general, but they are either email or username. If user doesn't know a lot about auth, they might also not be aware you can't log in to oauth providers like this. So it just feels confusing to have such a very general name at the top level of wasp/client/auth when it does something quite specific.

The rest is actually kind of ok.

TLDR

I guess on part of me wants to have things nicely organized, so when using them, one understand what is related to what, what operates on what, ... . But the only way I can see that achieved is by having multiple imports with subpaths, and I understand not everybody thinks that is a good DX. And maybe that is not really as important as I think it might be, so in that case ok, it might be worth trying going with this flat approach.

What I do think might be worth doing is pimping up names just a bit, to give some names that context that is missing. And I agree with preferring short names vs Java like names, but there are probably a couple of names that we could improve anyway:

  • CustomizationOptions
  • FormError, FormInput, ...
  • login / signup -> solution might be different then changing name though

Martinsos avatar Jan 30 '24 14:01 Martinsos

Related convos:

https://github.com/wasp-lang/wasp/pull/1693#discussion_r1471092890

https://github.com/wasp-lang/wasp/pull/1693#discussion_r1471098288

Martinsos avatar Jan 30 '24 15:01 Martinsos

I feel like having multiple ways of importing same symbols could be confusing to users e.g. import { LoginForm } from 'wasp/client/auth' and import { LoginForm } from 'wasp/client/auth/email'. But I understand the position that having scoped imports makes it maybe easier to reason about what is LoginForm that is used in the file.

I propose that we start with minimal API e.g. only wasp/client/auth and then we can see how it feels to us and our users. Later on, we can add new ways to import stuff. But if we now offer the extra ways to import stuff, taking it away later would be a breaking change.

infomiho avatar Jan 30 '24 19:01 infomiho

@infomiho said we could prefixCustomizationOptions and FormError, Form... with AuthUi, so we have AuthUiCustomizationOptions, AuthUiFormError, ... . I agree that might be a good option, that gives those names much needed context.

Martinsos avatar Jan 31 '24 09:01 Martinsos