-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix the auth helpers to only require fields they need from AuthIdentity
#2057
Conversation
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
@@ -130,6 +132,8 @@ const TaskView = ({ task }: { task: Task }) => { | |||
} | |||
} | |||
|
|||
const email = getEmail(task.user) |
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.
Using the helper in the headless tests.
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.
What do you mean 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.
Using the getEmail
helper in headless tests app to test it in headless tests :)
@@ -30,7 +30,7 @@ genAuth spec = | |||
Just auth -> | |||
-- shared stuff | |||
sequence | |||
[ genUserTs | |||
[ genFileCopy [relfile|auth/user.ts|] |
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.
No need for templating anymore for this file, I've used a copy file draft instead.
AuthIdentity
AuthIdentity
AuthIdentity
return tasks | ||
} | ||
}) satisfies GetTasks<void> |
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.
How come the frontend type tests aren't complaining about this?
Different todoApp. Boy, they really are diverging 😅
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.
This is always a good thing to do in TypeScript (use the structural type system define arguments to have as few constraints as possible).
There's also an item in Effective TypeScript that talks about it.
One thing I'd possibly like changed. If the MinimalUserEntityWithAuth
is the type users will be seeing the most of, I recommend flipping the naming:
- Change
MinimalUserEntityWithAuth
toUserEntityWithAuth
- Change
UserEntityWithAuth
to something more verbose.
Good point! The tricky part with the The problem is that the user will see this wacky (albeit correct) name, and we want to do better. Some alternatives:
I maybe like the first option the best since it's just a |
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.
Exactly what I had in mind, nice work!
@@ -49,7 +51,7 @@ function makeAuthUser(data: AuthUserData): AuthUser { | |||
}; | |||
} | |||
|
|||
function findUserIdentity(user: UserEntityWithAuth, providerName: ProviderName): {= authIdentityEntityName =} | null { | |||
function findUserIdentity(user: UserEntityWithAuth, providerName: ProviderName): UserEntityWithAuth['auth']['identities'][number] | null { |
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.
Nice!
@@ -130,6 +132,8 @@ const TaskView = ({ task }: { task: Task }) => { | |||
} | |||
} | |||
|
|||
const email = getEmail(task.user) |
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.
What do you mean here?
The auth helpers like
getEmail
andgetUsername
didn't work unless you provided the full user object with fullAuthIdentity
objects in theidentities
array.Given that we suggest to users to include only the
providerName
andproviderUserId
when manually fetching the user + auth + identities, we should make sure our auth helpers work in that scenario.This PR:
providerName
andproviderUserId
when using the helpers.