edgedb / edgedb-js

The official TypeScript/JS client library and query builder for EdgeDB
https://edgedb.com
Apache License 2.0
514 stars 65 forks source link

add auth support for solid start #1058

Open Huliiiiii opened 4 months ago

Huliiiiii commented 4 months ago

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[bot] commented 4 months ago

All commit authors signed the Contributor License Agreement.
CLA signed

Huliiiiii commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

https://github.com/solidjs/solid-start/issues/1512#issuecomment-2161045705 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).

scotttrinh commented 4 months ago

https://github.com/solidjs/solid-start/issues/1512#issuecomment-2161045705

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 commented 4 months ago

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.