Skip to content

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.

@gel-data-cla
Copy link

gel-data-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:
https://github.com/edgedb/edgedb-js/blob/f8e5af2adf01ee152e4b8a88c4e8f1eabc2f6d46/packages/auth-solid-start/src/server/index.ts#L957-L959
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
@scotttrinh
Copy link
Collaborator

Sorry we've been sitting on this so long! I'll get back to this in a bit, sorry for letting it slip between the cracks.

@Huliiiiii Huliiiiii closed this Aug 9, 2025
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