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

feat: throw on error types #494

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

Conversation

mmkal
Copy link

@mmkal mmkal commented Nov 2, 2023

What kind of change does this PR introduce?

Re supabase/supabase-js#885 and supabase/supabase-js#801 (would likely need a small change to supabase-js too once this is published, to close them)

What is the current behavior?

  1. throwOnError() has no effect at compile-time
  2. There's no way to set a client to always throw on errors

See issues above.

What is the new behavior?

The return type of a .throwOnError()'d query is always a "success" response at compile time. i.e. error is always null and data isn't unioned with null (though it can still be null for .maybeSingle() etc.)

Breaking change (albeit fairly small one): .throwOnError() now has to be either at the "start" or the "end" of a chain. This is no longer allowed:

await postgrest.from('users').select('*').throwOnError().single()

It would have to be either:

await postgrest.from('users').select('*').single().throwOnError()

or the client can be configured to have all queries throw on error:

const strictClient = postgres.throwOnError()
await strictClient.from('users').select('*').single()

Additional context

The motivator for me wanting this is the ugliness of littering supabase codebases with go-like err != nil checks:

const {data, error} = await db.from('users').select('*').eq('id', foo).single()

if (error) {
  throw error
}

console.log(`Hello ${data.username}`)

It gets even worse if you need multiple queries in the same scope. It's becomes awkward to alias data to anything meaningful, because then you also need to

const {data: user, error: userError} = await db.from('users').select('*').eq('id', foo).single()

if (userError) {
  throw userError
}

console.log(`Hello ${data.username}`)

const {data: orders, error: ordersError} = await db.from('orders').select('*').eq('user_id', user.id)

if (ordersError) {
  throw ordersError
}

You also need to avoid lint rules that get angry at throwing a "raw" value without re-wrapping.

This would allow doing export const createDBClient = () => createClient(...).throwOnError() in a single helper, then all queries would have the throwy behaviour. The above example would become:

const {data: user} = await db.from('users').select('*').eq('id', foo).single()

console.log(`Hello ${data.username}`)

const {data: orders} = await db.from('orders').select('*').eq('user_id', user.id)

Implementation

It ended up being a bigger change than I'd hoped, because the ThrowOnError typearg needed to be passed around everywhere. The breaking change was needed because the old throwOnError() implementation had a return type of this - and as far as I know there's no such concept in TypeScript as this but with typearg ThrowOnError = true. So, an abstract class can't have a method that returns a variant of its implementer - meaning the throwOnError method on PostgrestBuilder needs to return a PostgrestBuilder, not the concrete implementation via this. So it knows about the Result type but not about the rest of the fanciness on PostgrestTransformBuilder.

It might be possible to keep the old put-throwOnError-anywhere behaviour, but it would require quite a bit more fiddling and this seemed like a big enough change for me on a repo I'm not familiar with.

@mmkal mmkal mentioned this pull request Nov 2, 2023
@mmkal mmkal changed the title Throw on error types feat: throw on error types Nov 2, 2023
@mmkal
Copy link
Author

mmkal commented Nov 9, 2023

CC @soedirgo @steve-chavez @wyozi any thoughts on this?


Copying my comment on supabase/supabase-js#801

I got impatient so I published it under @rebundled/postgrest-js. Here's how you can use it with @supabase/ssr:

npm install @rebundled/postgrest-js
import { CookieOptions, createServerClient } from '@supabase/ssr'
import { PostgrestClient } from '@rebundled/postgrest-js'
import { cookies } from 'next/headers'
import { Database } from '~/db'

export const createClient = () => {
  const client = createServerClient<Database>(
    process.env.NEXT_PUBLIC_SUPABASE_URL!,
    process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!,
    {
      cookies: {
        get(name: string) {
          return cookieStore.get(name)?.value
        },
      },
    },
  )

  // @ts-expect-error `.rest` is protected
  const { rest } = client
  const wrapper = new PostgrestClient<Database>(rest.url, {
    fetch: rest.fetch,
    headers: rest.headers,
    schema: rest.schemaName,
  }).throwOnError()
  return Object.assign(wrapper, {
    rest,
    realtime: client.realtime,
    auth: client.auth,
  })
}

You can then use like:

import {createClient} from '..'

export default async () => {
  const client = createClient()

  const { data: user } = await client.from('users').select('*').single()

  console.log(`Hello ${user.username}`) // no need to check truthiness/ throw manually
}

If you need access to the non-throwing client, you can use createClient().rest. Note that if the PR is merged, it'll probably make sense to make a change in the supabase-js too, so that explicitly creating a new PostgrestClient wouldn't be necessary. But this seemed like a non-invasive way to let people try this out now.

@wyozi
Copy link
Contributor

wyozi commented Nov 9, 2023

It seems like a good change that I would find useful, but then again I'm a mere contributor :) Seems like Supabase team is not responding to PRs very quickly at the moment, which is the reason why there's e.g. 3 separate spread operator PRs open at the moment.

@AlbertMarashi
Copy link

+1 on getting this reviewed & merged plz

@mmkal
Copy link
Author

mmkal commented Jun 4, 2024

Tagging some more people who seem to have been reviewing PRs and commiting code etc., indicating to me this project is not abandoned: @soedirgo @abhadu

This PR only really affects types, and adds new type tests to make sure there aren't new regressions and that the types work correctly. It's also a big quality-of-life improvement so I think it's worth taking a look!

I've updated from master, but it was fairly painful resolving the merge conflicts. This was work that would not have existed if this had gone in before @soedirgo's change f91aa29 - and I would rather not have to keep resolving merge conflicts if all that needs to happen is for this to be reviewed!

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.

3 participants