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.86k stars 220 forks source link

Add type definitions. #18

Closed thorwebdev closed 3 years ago

thorwebdev commented 3 years ago

This is a WIP draft. Feedback and contributions are very welcome! This relates to https://github.com/supabase/supabase/issues/122

interface Message {
  id: number;
  inserted_at: string;
  message: string;
  channel_id: number;
  author?: { name: string };
}

// Resolves with messages: any
const { body: messages } = await supabase
  .from('messages')
  .select('some_column, other_column')

// Resolves with messages: Array<Message>
const { body: messages } = await supabase
  .from<Message>('messages') // Message is the type of row in database
  .eq('channel_id', 1) // IDE will be able to help with the completion of channel_id
  .select(`*, author:user_id(id, username)`) // Not sure if we can do much autocompletion here
  .order('inserted_at', true); // IDE will help with autocompletion
thorwebdev commented 3 years ago

@kiwicopple when I run npm run build babel ignores the index.d.ts file. Do you know how we can get babel to copy it over to the lib folder?

Found it, needed to add the --copy-files option.

malobre commented 3 years ago

All functions defined in the Auth interface may fail for various reasons (wrong credentials, couldn't connect to server, etc) and return a ResponseError.

kiwicopple commented 3 years ago

Note on the failing integration tests - this can be ignored. We will have to roll the GoAuth middleware into a new docker image so that it can be run locally.

We will solve that separately.

@thorwebdev i'll trust your lead here - it's a non-breaking change so I am happy for you to merge this whenever you're happy. Once it's merged it will be much easier for the community to contribute patches (like the one malobre has requested).

I'm happy to merge if you want too :shipit: You're about to make it onto our humans.txt

thorwebdev commented 3 years ago

@malobre the Auth methods don't return the error, but rather throws it which is something you have to handle with a try catch. However this is a good point that this has to be documented here: https://supabase.io/docs/library/user-management

thorwebdev commented 3 years ago

@kiwicopple IIRC @awalias wanted to first change the chaining hierarchy in the docs and the dashboard to follow the chaining hierarchy that the types use, e.g. supabase.from().select().filter().single(), to make copy and pasting from the docs for TypeScript users more convenient. But I'm happy to not block on that and rather do that as a fast follow, wdyt?

From my pov this is peaceful to merge 👍

malobre commented 3 years ago

@malobre the Auth methods don't return the error, but rather throws it which is something you have to handle with a try catch. However this is a good point that this has to be documented here: https://supabase.io/docs/library/user-management

Nice to know, I wasn't sure how typescript handles thrown errors. Anyway, the type definition is working fine in my personal projects :)

soedirgo commented 3 years ago

@thorwebdev IIRC it was because some postgrest-js methods are missing in supabase-js (limit, or), which https://github.com/supabase/supabase-js/pull/13 is supposed to fix. This delegates the postgrest-js functionality to... postgrest-js, but this means we'll have to wait for the postgrest-js TS transition to get the types.

I personally prefer doing select()/insert()/update() last, but we should let people do both styles.

thorwebdev commented 3 years ago

@soedirgo while it seems desirable to be able to call any of the methods at any time, I was worried about the cognitive load (Working memory can generally hold between five and nine items (or chunks) of information at any one time.), and therefore tried to model the major decision tree:

This way you can work with the supabase client without needing to consult documentation. The order is only enforced in TypeScript, so any existing JavaScript code will continue working, but I personally think that having a chaining order is generally a good idea, at least for people who don't have all of the available methods memorized. Wdyt?

soedirgo commented 3 years ago

You're right, I didn't think of it from a text completion PoV. That way of doing it is more in line with the intended usage.

But say we convert supabase-js to TS. Is it possible to allow arbitrary order of chaining without the compiler yelling? Otherwise I prefer to enforce this order even in JS, since there should at least be method completion even w/o types. This is the best time to do it since I'm working on converting postgrest-js to TS. This will break some code though...

I still think these methods should only be in postgrest-js (select, filters, etc.). That way whenever I make a (non-breaking) change on postgrest-js I don't need to update supabase-js.

thorwebdev commented 3 years ago

@soedirgo I had a look at how knex.js does this, and it's a bit of a mess IMO: https://github.com/knex/knex/blob/master/types/index.d.ts https://lorefnon.tech/2019/05/02/using-boxing-to-prevent-distribution-of-conditional-types/

I personally would be in favor of being opinionated about the chaining order across both JS and TS as it allows us to be coherent across docs and samples, and I think it's a better DX, but also understand that this might be a very subjective opinion. So would need @awalias and @kiwicopple to chime in on this.

I still think these methods should only be in postgrest-js (select, filters, etc.). That way whenever I make a (non-breaking) change on postgrest-js I don't need to update supabase-js.

Yes, absolutely on board with that 👍

kiwicopple commented 3 years ago

We'll have to take a utilitarian approach to developer happiness here. Looks like we're trading flexibility for better/easier docs.

I think the latter gives a better developer experience, so I'm happy to accept Thor's changes & force opinionated chaining.

This is the best time to do it since I'm working on converting postgrest-js to TS. This will break some code though...

Also we will have to update some of our docs. @soedirgo if you're on board & so is Ant, let's do it. If you think it needs to wait until I'm back to coding, then I can jump into this in ~2 weeks