Skip to content

Add type definitions. - #18

Merged
thorwebdev merged 12 commits into
masterfrom
feat/add-type-definitions
Aug 17, 2020
Merged

Add type definitions.#18
thorwebdev merged 12 commits into
masterfrom
feat/add-type-definitions

Conversation

@thorwebdev

@thorwebdev thorwebdev commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

This is a WIP draft. Feedback and contributions are very welcome! This relates to supabase/supabase#122

  • createClient
  • supabase.auth
  • Add detailed comments to AuthResponse and User params
  • Type all other SupabaseClient functions
  • Figure out how to weave in your custom data types
    e.g. this could look like
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
thorwebdev marked this pull request as draft August 10, 2020 19:36
@thorwebdev
thorwebdev marked this pull request as ready for review August 12, 2020 15:43
@thorwebdev thorwebdev changed the title [WIP] Add type definitions. Add type definitions. Aug 12, 2020
@thorwebdev
thorwebdev requested a review from kiwicopple August 12, 2020 15:44
@thorwebdev

thorwebdev commented Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

@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

malobre commented Aug 17, 2020

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Contributor Author

@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

malobre commented Aug 17, 2020

Copy link
Copy Markdown

@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 :)

@thorwebdev
thorwebdev merged commit dedf5ee into master Aug 17, 2020
@thorwebdev
thorwebdev deleted the feat/add-type-definitions branch August 17, 2020 14:10
@soedirgo

Copy link
Copy Markdown
Member

@thorwebdev IIRC it was because some postgrest-js methods are missing in supabase-js (limit, or), which #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

Copy link
Copy Markdown
Contributor Author

@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:

  • 1st decision: I want to perform some operation on a table
    image
  • 2nd decision: I want to read some data
    image
  • 3rd decision: I want to apply some filters / limits
    image
  • 4th decision: I want to modify the response shape (single item)
    image

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

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.

4 participants