feat: typecheck table names in from clause

supabase / supabase-js

An isomorphic Javascript client for Supabase. Query your Supabase database, subscribe to realtime events, upload and download files, browse typescript examples, invoke postgres functions via rpc, invoke supabase edge functions, query pgvector.

https://supabase.com

MIT License

2.83k stars 219 forks source link

feat: typecheck table names in from clause #984

Closed vonovak closed 1 month ago

vonovak commented 2 months ago

What kind of change does this PR introduce?

Please take this as a sort of a question because I'm new to supabase. (and I'm aware of the // NOTE: signatures must be kept in sync with PostgrestClient.from).

Motivation

I want to query supabase using the client and want to do that with the best possible guidance from TS.

Right now, TS (although it will suggest valid table names) won't complain if I try to query a non-existent table. To me, this is a fairly unexpected limitation and I don't understand why it's there.

So I'm suggesting this change here (I can also make it in the other place) with the aim of achieving higher code safety. If this is not possible, why? (Are there other cases like this, with low-hanging fruit for TS improvements?)

Thank you! :)

kamilogorek commented 1 month ago

Hey, this looks like a good addition. We did this previously because we needed a support for any generic name, due to materialized views not being supported in the generated types. Now that they are, we can update the types just fine.

As for your change, the only required one is the last line, which is removal of the 3rd overload. We need to use extends string & keyof instead of just extends keyof, as symbol type can also be passed as a type without that restriction, and it'll fail inside postgrest-js, where we do string interpolation.

So now to make it land, what we need is:

change the PR to only include last overload removal
updated postgrest-js to make it inline with this change
bump postgrest-js version here

Thanks for catching this one! (not that I'll be OOO for the next 2 weeks, so can be slow to respond) cc @soedirgo

soedirgo commented 1 month ago

We also need to support generating types for foreign tables: https://github.com/supabase/postgres-meta/pull/754

Otherwise, this change looks good to me - will take care of the typegen updates and postgrest-js changes 👍

vonovak commented 1 month ago

@soedirgo I made the necessary change 🙂

soedirgo commented 1 month ago

Thanks! The typegen support for foreign tables has been deployed, so merging this in 🚀

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 8395866041

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 65.55%

Totals
Change from base Build 7957284757:	0.0%
Covered Lines:	89
Relevant Lines:	116