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

add auth support for solid start #1058

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

Huliiiiii
Copy link

template
Based on auth-nextjs and auth-remix, replace functions with corresponding alternatives. Tested on the template, capable of normal sign up, sign in, add and delete items, but no further extensive testing has been conducted.

@edgedb-cla
Copy link

edgedb-cla bot commented Jul 18, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@Huliiiiii Huliiiiii marked this pull request as ready for review July 18, 2024 08:16
- formatting
- clean up deps
@Huliiiiii
Copy link
Author

Huliiiiii commented Jul 18, 2024

There are some type issues in CreateAuthRouteHandlers (redirect function in solid router return a response, but other router maynot do the same thing), is replacing return types with unknown (or Response | never) a good solution?

@scotttrinh
Copy link
Collaborator

Amazing, thanks so much for the contribution! I'll take a closer look at it later today, but based on my initial light pass, it seems solid 😉

There are some type issues in CreateAuthRouteHandlers (redirect function in solid router return a response, but other router maynot do the same thing), is replacing return types with unknown (or Response | never) a good solution?

It seems idiomatic to throw using redirect (see https://docs.solidjs.com/solid-router/reference/data-apis/response-helpers#redirect) so I think it's fine to keep never here? Can you share some code that you have that shows the type error you're seeing?

@Huliiiiii
Copy link
Author

Huliiiiii commented Jul 18, 2024

solidjs/solid-start#1512 (comment)
Actually, throw redirect is handle by wrappers like:

try {
  // do something
} catch (err) {
  if (err instanceof Response) {
    return err
  } else {
    throw err
  }
}

If the code is not wrapped by a wrapper, it will cause an error. This may be a solid start issue. Therefore, it is safer to return a redirect if needed.

export const { GET, POST } = serverAuthHelper.createAuthRouteHandlers({
  async onBuiltinUICallback({ error, tokenData, isSignUp }) {
    "use server"
    if (error) {
      console.error("Authentication failed: ", error)
      return redirect("/error?error=auth-failed")
    }

    if (!tokenData) {
      console.error("Email verification required.")
      return redirect("/error?error=email-verification-required")
    }

    if (isSignUp) {
      const client = serverAuthHelper.getSession({
        tokenData,
      }).client

      const emailData = await client.querySingle<{ email: string }>(`
        SELECT ext::auth::EmailFactor {
          email
        } FILTER .identity = (global ext::auth::ClientTokenIdentity)
      `)

      console.log("emailData: ", emailData)

      await client.query(`
        INSERT User {
          name := '',
          email := '${emailData?.email}',
          userRole := 'user',
          identity := (global ext::auth::ClientTokenIdentity)
        }
      `)
    }

    return redirect("/")
  },
  async onSignout() {
    "use server"
    throw redirect("/")
  },
})

By the way, here's some code from react that needs to be cleaned up (e.g. handle actions, I didn't figure out how it works).

@Huliiiiii Huliiiiii marked this pull request as draft July 18, 2024 14:04
@scotttrinh
Copy link
Collaborator

solidjs/solid-start#1512 (comment)

Ahh, I see. Well, it's fine to return Response | never here! I never really liked the "throw redirects" style that other frameworks have, in my opinion it's more clear if functions actually return values rather than opt out of the type system and normal control flow via throw.

By the way, here's some code from react that needs to be cleaned up (e.g. handle actions, I didn't figure out how it works).

Can you point out what needs to be cleaned up? That looks like example code, not library code.

@Huliiiiii
Copy link
Author

Huliiiiii commented Jul 18, 2024

solidjs/solid-start#1512 (comment)

Ahh, I see. Well, it's fine to return Response | never here! I never really liked the "throw redirects" style that other frameworks have, in my opinion it's more clear if functions actually return values rather than opt out of the type system and normal control flow via throw.

By the way, here's some code from react that needs to be cleaned up (e.g. handle actions, I didn't figure out how it works).

Can you point out what needs to be cleaned up? That looks like example code, not library code.

I mean the library (sorry for my English).

The part need to clean up is:

function _isAction(data: any) {
return typeof data === "object" && data._action === true;
}

This may always be false. I think just need to remove them.

@Huliiiiii Huliiiiii marked this pull request as ready for review July 18, 2024 15:22
- make package name consistent with others
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.

2 participants