Skip to content
This repository was archived by the owner on Oct 9, 2025. It is now read-only.

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 force-pushed the throwOnError-types branch from 57323f8 to 204cdf1 Compare November 2, 2023 04:41
@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!

@mandarini
Copy link
Contributor

Hi @mmkal ! Thanks for the contribution and your patience.

This repository is deprecated and has moved to the new Supabase JS monorepo. Since this PR has been inactive for over a year, I’m going to close it to keep the old repo tidy, before archiving.

If you believe this change is still needed, please open a new PR in the monorepo and include a link back to this thread for context:

Note: This old repository will be archived on October 10, 2025. Thank you again for your effort and understanding!

I understand you put work into this, and I will try my best to see how we could import this work in the new repository. Again thank you for your understanding.

@mandarini mandarini closed this Oct 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants