-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eng(supabase): add supabase and offline first with supabase packages #403
Conversation
this.query, | ||
}) : adapter = modelDictionary.adapterFor[_Model]!; | ||
|
||
String get selectQuery { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devj3ns This is maybe the secret sauce to getting a single, efficient queries instead of a single request for every nested association. I haven't written any tests yet. If you're keen, I'd like to collaborate with you on making this robust.
The big idea is Brick's query system needs to match Supabase's API / their own internal query system from the Flutter package.
The smaller picture is
- Writing tests for
Compare
cases that build a URI. This is the area I could most use your help, since you've been dealing with this most recently and know a valid/invalid URI - Writing tests for
selectQuery
that covers associations - Writing tests for `selectQuery that cover nested associations up to four levels
- Writing tests for
selectQuery
that handleforeignKey
config to map to the right tables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tagging me @tshedor, I will take a look at it in the coming days and try to help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devj3ns I'm going to merge this now so that everything is on the same branch. We can do smaller PRs with much less boilerplate to start polishing the lower-level work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tshedor, I just wanted to start writing tests for the transformer, but I struggle to get the constructed URI back from the QuerySupabaseTransformer
.
To my understanding, I need to create transformers with sample queries like this:
final transformer = QuerySupabaseTransformer<DemoModel>(
modelDictionary: demoModelDictionary,
query: Query.where('name', 'Thomas'),
);
And then check if the constructed URI is correct. But how do I get the URI from the transformer? I tried using select
but the PostgrestFilterBuilder
does not allow me to get the URI.
expect(?, equals('name=eq.Thomas'));
Could you please guide me in the right direction? I am sure I am currently unseeing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me @tshedor 👍
I have not opened the code in my IDE yet, but read through it and added some comments/questions.
### `@Supabase(enumAsString:)` | ||
|
||
Brick by default assumes enums from a REST API will be delivered as integers matching the index in the Flutter app. However, if your API delivers strings instead, the field can be easily annotated without writing a custom generator. | ||
|
||
Given the API: | ||
|
||
```json | ||
{ "user": { "hats": ["bowler", "birthday"] } } | ||
``` | ||
|
||
Simply convert `hats` into a Dart enum: | ||
|
||
```dart | ||
enum Hat { baseball, bowler, birthday } | ||
|
||
... | ||
|
||
@Supabase(enumAsString: true) | ||
final List<Hat> hats; | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: When enums (Enumerated Types) are stored and returned by their name and not the index in Postgres/PostgREST, why is this option necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devj3ns this option is necessary for exactly that case. By default, enums are serialized/deserialized by their index, and enumAsString
uses the name instead
|
||
@SupabaseSerializeable(tableName: 'customers') | ||
class Customer { | ||
final int id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest changing this to type String or UUID, to avoid collisions. As recommended in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent catch, updated
this.query, | ||
}) : adapter = modelDictionary.adapterFor[_Model]!; | ||
|
||
String get selectQuery { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tagging me @tshedor, I will take a look at it in the coming days and try to help!
Adding to #401