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

Export SignupHandlerOptions interface and update its userAttributes type #9049

Merged
merged 7 commits into from Dec 19, 2023
Merged

Export SignupHandlerOptions interface and update its userAttributes type #9049

merged 7 commits into from Dec 19, 2023

Conversation

c-ciobanu
Copy link
Contributor

@c-ciobanu c-ciobanu commented Aug 20, 2023

This sparkled from a discussion in the community forum here https://community.redwoodjs.com/t/signuphandleroptions-userattributes-type/5231/2

Problem:

Since the type of userAttributes is Record<string, string> and if you pass something that is not a string to your signupHandler you are forced to convert types to stop TS complaing.
e.g.

handler: ({ username, hashedPassword, salt, userAttributes }) => {
  const { items, age } = userAttributes as unknown as { items: number[], age: number }
  const relatedItems = items.map((item) => ({ id: item }))
  ...
}

Solution:

  • export SignupHandlerOptions so you can do something like this:
    handler: ({
      username,
      hashedPassword,
      salt,
      userAttributes,
    }: SignupHandlerOptions & { userAttributes: { items: number[], age: number } }) => {
      const relatedItems = userAttributes.items.map((item) => ({ id: item }))
      ...
    }
  • update the type of userAttributes to Record<string, any> so you could just write const relatedItems = userAttributes.items.map((item) => ({ id: item })) without TS complaining or you could write this too without having to convert to unknown first
    handler: ({ username, hashedPassword, salt, userAttributes }) => {
      const { items, age } = userAttributes as { items: number[], age: number }
      const relatedItems = items.map((item) => ({ id: item }))
      ...
    }

There was an idea about having userAttributes as Record<string, unknown> but I personally think that by doing that the second point of the solution would not to possible anymore and you would be forced to use the first solution. Any comments about that?

@thedavidprice
Copy link
Contributor

Thanks for this @c-ciobanu! @cannikin Any chance you can take a look?

@cannikin
Copy link
Member

@dac09 helped me figure out those types, I really don't know what The Right Way is to type all of this stuff. I think what you're saying makes sense, but I'm no TS expert. Anyone else want to weigh in? @dac09 or @Tobbe maybe?

@Tobbe
Copy link
Member

Tobbe commented Aug 30, 2023

My knee jerk reaction is Record<string, unknown>. Really no fan of any 😅

But actually I thought of a slightly different solution. What if we make DbAuthHandler take another generic type in addition to the existing TUser? Call it TUserAttributes and then pass it down to DbAuthHandlerOptions and SignupFlowOptions. Would that work?
(When trying that option out I recommend temporarily removing the default type from TUser and adding TUserAttributes without a default type too. When we know it all works we can add those types back in again for people who can't be bothered with proper types 😛 )

@c-ciobanu
Copy link
Contributor Author

@Tobbe
I'm not sure I understood completely your intention but I kind applied your suggestion and passed a new type TUserAttributes to DbAuthHandler and then pass it down.
I didn't make it optional since that by doing so everyone will need to add it and I don't think that is what we want.

With this change now I can do this basically:

const signupOptions: DbAuthHandlerOptions<
  undefined,
  { age: numbers; items: number[] }
>['signup'] = {
  ...
  handler: ({ username, hashedPassword, salt, userAttributes }) => {
  ...

userAttributes will have the correct types in this case.

@Tobbe Tobbe self-assigned this Dec 17, 2023
@Tobbe Tobbe removed the request for review from cannikin December 17, 2023 12:19
@Tobbe Tobbe added the release:feature This PR introduces a new feature label Dec 19, 2023
@Tobbe Tobbe added this to the next-release milestone Dec 19, 2023
@Tobbe Tobbe enabled auto-merge (squash) December 19, 2023 10:38
@Tobbe Tobbe merged commit 76632d0 into redwoodjs:main Dec 19, 2023
32 checks passed
@c-ciobanu c-ciobanu deleted the user-attributes-type branch December 19, 2023 15:08
Tobbe added a commit that referenced this pull request Dec 21, 2023
…ype (#9049)

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@Tobbe Tobbe modified the milestones: next-release, v6.6.0 Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants