feat: add generic types to .select()

[Vinzent03]

The idea is to pass a type to from select, which sets the response type. I had to add another generic to PostgrestBuilder to properly type the converter. The type from from is used as parameter type for converter. This doesn't infer the type depending on single or count, but makes it easier to type if you know what you get. insert/update/upsert/delete now return void and need a select to change the type.

close supabase/supabase-flutter#252

[dshukertjr]

This looks amazing! It seems like this PR is still in Draft mode, but is it completed?

Also, maybe you plan to do this already, but if you could write how the public API is going to change (example code on how you can pass types to from), that would be amazing!

[Vinzent03]

I converted it to draft to adapt the tests to the new system.

[dshukertjr]

Looking forward to this one! I really like the new API!

final data = await client
    .from<List<Map<String, dynamic>>>('countries')
    .select()
    .withConverter<List<Country>>(
        (data) => data.map(Country.fromJson).toList());

[Vinzent03]

It gets even better by adding typedefs for common return types.

[Vinzent03]

Okay turns out this is much more complex than I thought, because you can't cast a List<dynamic> to List<String>, but you have to use List.from(). I forgot that first. In addition, T is currently often used as type for PostgrestResponse.data, but T is now the overall return type, including PostgrestResponse. I'm still trying to figure things out, but this might not be as easy as first thought.

[Vinzent03]

@dshukertjr Should be working now. You can only use the specific typedefs, because we have to manually cast the data to the type. I think we should explain under which circumstances they can expect which type. Where should we add this? As well, under from?

[Vinzent03]

The case of insert/update/delete without select is still not possible, because Null is always inferred to dynamic>. dart-lang/language#436

Another proposal

We could make it void though, but I don't know if this is a breaking change.

We could make from to return void by default and only allow the generic on select, because without select it's always void/null, right? Returning always null is a bit useless, but I don't know if we want to make this void.

final data = await postgrest.from('users').insert({'username': 'bar'});
//data is void

final data = await postgrest.from('users').insert({'username': 'bar'}).select<PostgrestList>();
//data is of type PostgrestList

[dshukertjr]

We could make from to return void by default and only allow the generic on select, because without select it's always void/null, right? Returning always null is a bit useless, but I don't know if we want to make this void.

I love this. Let's do it. I don't think it's a breaking change.

[dshukertjr]

You can only use the specific typedefs, because we have to manually cast the data to the type. I think we should explain under which circumstances they can expect which type. Where should we add this? As well, under from?

Frankly, I'm not sure if I'm a huge fan of introducing several new typedefs in a public API. I think it could cause more confusions than solving it. Could we just do something like this:

final data = await supabase.from<List<Map<String, dynamic>>>('table').select();

Edit

I had a misunderstanding on how typedef works, and noticed that above code is valid with the current codebase!

[Vinzent03]

Great, will push the new proposal later today.

[dshukertjr]

So it looks like the final api we have is like this?

final data = await supabase.from('table').select<List<Map<String, dynamic>>>();

At first, I thought this might be a crazy idea, but I wonder if this would be a better solution for this issue that we are trying to solve.

We could define two more classes PostgrestListResponse and PostgrestMapResponse that implements List and Map respectively.

class PostgrestListResponse extends PostgrestResponse implements List<Map<String, dynamic>> {
  ...
}

class PostgrestMapResponse extends PostgrestResponse implements Map<String, dynamic> {
  ...
}

With this, we could

final data = await supabase.from('table').select(); // data here has `PostgrestListResponse`

final firstRow = data.first; // This is valid since `PostgrestListResponse` implements List<Map<String, dynamic>>

[Vinzent03]

@dshukertjr You are right, this may work. I would still keep insert/... as PostgrestResponse<void> then. The default generic from select would be PostgrestListResponse. single and maybeSingle would change the generic to PostgrestMapResponse. But the methods define the generic, and the user won't be able to specify any generic themselves, right? Will have to update all the tests again then... 🤣

[Vinzent03]

How should we handle maybeSingle. It returns a PostgrestResponse, but with data being null. I can't return null from e.g. containsKey(), because that's not a correct override. We could throw an exception, but that's not helpful.

[dshukertjr]

@Vinzent03 It might be faster to discuss what to do with this PR. I have sent you a message on Discord. If you have time, let's discuss over a voice chat it there!

[dshukertjr]

After further thinking about it, I don't have any good idea on how we might be able to handle maybeSingle without a breaking change. I am happy to merge this one if you are! Thanks for the amazing work!

[dshukertjr]

@Vinzent03 Wanted to get one last approval to double check. I'm good with merging this one, but would it be okay on your end as well?

[Vinzent03]

@dshukertjr I've written a comment here. #94 (comment) I think we should change the doc more to demonstrate the "danger" of using the wrong type and that it's optional.

[dshukertjr]

I think we should change the doc more to demonstrate the "danger" of using the wrong type and that it's optional.

We could probably provide some example code for each return type with some description. Something like this maybe?

  /// - [List<Map<String, dynamic>>] is returned from query without `.single()` or `maybeSingle()` and does not contain `count` option.
  ///   ```dart
  ///   final data = await supabase.from('table').select();
  ///   ```
  /// - [Map<String, dynamic>] is returned from query with `.single()` and does not contain `count` option.
  ///   ```dart
  ///   final data = await supabase.from('table').select().single();
  ///   ```
  ... etc

[Vinzent03]

@dshukertjr I think the types are now properly documented on both select methods. What do you think?

[dshukertjr]

Love the docs! Let's merge this one in! Thanks for the amazing update!