Feedback about ferry HOT 23 CLOSED

Sorry for second comment, 2 more points:

1. A ClientProvider would allow the Query widget to get the client from context instead of forcing the developer to pass the client manually.
2. I would rename the flutter library to FlutterQL so it stands out from all those gql_* packages 🤘 (as flutter is probably the most prominent use case for this library)

from ferry.

Until gql-dart/gql#32 is merged, to run the example you will need to override the gql_build / gql_code_builder dependencies with my branch.

from ferry.

@smkhalsa This looks like some quality work! I particularly like the use of rxdart and hive.

• I would personally make all the Options types share an ancestor and make them value types (I really wish I knew about and started leveraging built_value earlier)
• good move decoupling store from cache
• idk what's going on with this self import: https://github.com/smkhalsa/gql_client/blob/92e1d71fff5961ad52ad73fa74809c6af4d53172/examples/gql_example_flutter/lib/fragments.gql.dart#L1

Other than that, I doubt I'll have the time to play around with it more thoroughly for a while. Cool stuff though!

from ferry.

Thanks @micimize for the input!

I would personally make all the Options types share an ancestor and make them value types (I really wish I knew about and started leveraging built_value earlier)

Yes, I'm definitely interested in exploring built_value, especially for the generated data classes (gql-dart/gql#35).

idk what's going on with this self import:

I know, I thought the same. For some reason, code_builder generates a self-import if a class from the same source file is referenced.

from ferry.

@smkhalsa, great work!

I am a fan of small focused packages, so I'd say the cache/store layer should be extracted. By following the issues on graphql-flutter project, I feel like that's the part which is not working very well there. So this could be a great addition to the tools the Dart GraphQL community has. Your work on normalize and this client makes me think you are the man for the job.

We should think about making the codegen part more user-friendly. Your suggestions at gql-dart/gql#36 might be one way to fix it. Or maybe we should take another direction - bundle the code from gql_code_builder into a dedicated build package for this client.

Also, if you have any feedback or concrete examples of how using built_value improves the generated code, please leave a comment in https://github.com/gql-dart/gql/issues

Again, great work! Sorry I cannot do a more detailed exploration of the client at this point.

from ferry.

I would personally make all the Options types share an ancestor and make them value types (I really wish I knew about and started leveraging built_value earlier)

@micimize done. Thanks for the suggestion.

from ferry.

@klavs Thanks for the feedback and encouragement!

I'd say the cache/store layer should be extracted.

I agree. I started working on it as a monorepo, but once the API stabilizes I intend on splitting the packages up into the following independent packages:

1. gql_cache
2. gql_store
3. normalize
4. gql_client
5. gql_client_builder (for my req_builder and any other client-specific builders)
6. gql_client_flutter (for Query widget)

We should think about making the codegen part more user-friendly. Your suggestions at gql-dart/gql#36 might be one way to fix it. Or maybe we should take another direction - bundle the code from gql_code_builder into a dedicated build package for this client.

Absolutely. Right now I'm writing my .graphql files in a graphql subdirectory, but this is still a bit of a mess. I'll think a bit more on this one.

Also, if you have any feedback or concrete examples of how using built_value improves the generated code, please leave a comment in https://github.com/gql-dart/gql/issues

The more I think about it, the more I feel that using built_value for data_builder (and possibly other builders) would be helpful. The immutability aspect will make it trivial to only emit distinct results in the responseStream for a particular query. Since watched queries depend on a single, global data stream, every update currently triggers ALL query streams to emit, even if the data for that query hasn't changed.

Additionally, the built_value rebuild method will make it trivial to make updates to data read from the cache. For example, if I have an AddPokemonMutation and use an UpdateCacheHandler to read an AllPokemonQuery from the cache, append the new pokemon, then write the query back to the cache, built_value would make this very easy.

Although these examples are specific to my client implementation, I feel that the underlying benefits are more broadly applicable.

from ferry.

I am a fan of small focused packages, so I'd say the cache/store layer should be extracted. By following the issues on graphql-flutter project, I feel like that's the part which is not working very well there

The real problem with graphql-flutter is that it doesn't really have a core maintainer. @mainawycliffe is the closest to one right now. I'm trying to laser-focus as much as I can on the app I'm building.

There are a few pieces of the graphql-flutter api particularly worth mentioning:

• QueryResultSource solves result source ambiguities in a scaleable way (i.e. is this result an optimistic response there's going to be another, or is it just loaded with cacheFirst?). I think that becomes more important with dynamic offline resolution
• structured exceptions to disambiguate between client errors and graphql errors
• the client-level policy system
• I'm not sure if you've eschewed this issue, but I ended up implementing MultiSourceResult for getting eager results synchronously

I actually like the concept of updateCacheHandlers way more than mutation(update: ...). My conceptualization of the ideal fullstack architecture involves just having a cache that has much of the same business rules as the server, making it essentially a replicated shard of the server-side database (like with couch/pouchdb).

from ferry.

@smkhalsa Looks like Awesome work khalsa ji😀, will take it out for a spin!!!

from ferry.

@JasCodes great, let me know if you have any feedback

from ferry.

https://github.com/smkhalsa/gql_client/blob/a95803add3c52e90b7fa578e32773292dfc3d262/lib/src/client/client.dart#L17-L20
@smkhalsa, I think there's no need to prefix the classes with GQL, because user is always able to namespace their imports if there's a conflict in their file, for example, gqlc.Client client

https://github.com/smkhalsa/gql_client/blob/a95803add3c52e90b7fa578e32773292dfc3d262/lib/src/client/query_response.dart#L16-L17
In my opinion, type-awareness should be built on top of the client instead of inside the client. Now that you've implemented significant part of the client, do you feel it's going to work well with type-awareness built-in?

Is there any particular reason, why QueryRequest and QueryResponse does not implement Request and Response from gql_exec or include them as fields?

Kudos to you for readable code!

from ferry.

I think there's no need to prefix the classes with GQL, because user is always able to namespace their imports if there's a conflict in their file

Good point. I've removed the prefixes.

In my opinion, type-awareness should be built on top of the client instead of inside the client. Now that you've implemented significant part of the client, do you feel it's going to work well with type-awareness built-in?

Interesting. @klavs, can you please expand on this a bit? Honestly, I've been thinking about going the other direction and adding type-awareness into the cache layer (or at least CacheProxy). Currently, when reading or writing data to the cache in an UpdateCacheHandler, the user can't pass a QueryRequest<T> directly to readQuery or writeQuery; they must create a ReadQueryOptions object, and the result of readQuery is not typed, so the user must manually instantiate a typed object with the given data or use the untyped data directly. Then they must create a WriteQueryOptions object to write the mutated data back to the cache. By adding type-awareness, the user could simply pass their QueryRequest<T> to readQuery and get T data back, then pass the same QueryRequest<T> to writeQuery to write data back to the cache.

Ideally, we'd also add a builder to generate FragmentRequest<T> objects to use with readFragment and writeFragment. Also, if data_builder implemented built_value, then it would be trivial to mutate T data using rebuild.

Is there any particular reason, why QueryRequest and QueryResponse does not implement Request and Response from gql_exec or include them as fields?

I thought about it (and am still open to it). However, since QueryResponse.data is typed, it doesn't correctly implement Response's interface.

from ferry.

@smkhalsa I think type aware cache would be such an improvement. In fact with I must you must be thinking implementing unified client side queries as well. Having single cache for device data and server data would be awesome!!!

from ferry.

@smkhalsa, never mind! I think I just misinterpreted the code. The underlying cache seems to be implemented as a low-level "untyped" storage while you've added the type-layer at the appropriate places.

from ferry.

I LOVE your work and appreciate it so much!
I would have my humble feedback here:

1. I think a builder that can read the graphql queries from the dart widgets files themselves instead of separate .graphql files will allow for (the good kind of-) data-widget coupling. This is why relay is such a powerful graphql client.
2. support for apollo-like persisted queries brings much benefit on so many fronts: security, performance, authorization and probably more.
3. making a fragmentbuilder widget like in react-relay for the flutter part allows for great data masking and modularity of the widget tree.

Cheers bro!

from ferry.

Hey @iscriptology!

I think a builder that can read the graphql queries from the dart widgets files themselves instead of separate .graphql files will allow for (the good kind of-) data-widget coupling. This is why relay is such a powerful graphql client.

I think the Dart build system is designed not to override contents of non-generated files. So the best solution to query inlining would still keep the string and ast representation of the query in memory.

Please let me know if there is a way to do what you've suggested.

from ferry.

@klavs hey
The PODOs should still rest in the generated files, but the input graphql strings should come from .dart files instead of .graphql files.

For example some pseudo code that comes to my mind:

@gql(“query MyQuery . . .) // this is the data requirememt for this widget
class MyWidget with MyQueryRequest extends ... Widget {
...
initState() {
    fetchQuery() . . .
}

build() {
    return MyFragment();
}

@fragment(“fragment UserFragment ...on User“)
class MyFragment . . .

Sorry for the style of the code I’m on my iPhone on the road ❤️

from ferry.

That's interesting... If Dart strips annotations from the build, it might be the way to go.

from ferry.

As I understand it annotations are specifically designed for code generation scenarios, so dart probably does strip them.
I would love to help with this effort, @smkhalsa, @klavs LMK if it's on your roadmap.

from ferry.

I think the best first step is to write an example code with some edge cases showing how you'd want it to behave.
When that's done we'll be able to evaluate how to best do it maintaining the ability to generate from query files if needed.

from ferry.

@iscriptology thanks for the feedback!

I think a builder that can read the graphql queries from the dart widgets files themselves instead of separate .graphql files will allow for (the good kind of-) data-widget coupling.

I agree that (at least for Flutter) defining queries in the same file as the Widget using an annotation is probably cleaner. Can you think of any downsides to this approach?

One benefit of separate .graphql files is that there's already an ecosystem around .graphql files that you can plug into which provides useful functionality in IDEs such as syntax highlighting / validation / formatting.

support for apollo-like persisted queries brings much benefit on so many fronts: security, performance, authorization and probably more.

I assume you're referring to Apollo's Automatic persisted queries? I haven't used this feature, but it seems like this is tightly coupled with Apollo's server implementation. What changes to the client would be needed to implement this?

making a fragmentbuilder widget like in react-relay for the flutter part allows for great data masking and modularity of the widget tree.

I believe this relates to #3, so please see my comments there.

A ClientProvider would allow the Query widget to get the client from context instead of forcing the developer to pass the client manually.

I thought about this, but I didn't want to be opinionated about dependency injection. I recently switched from using provider to using get_it, and it doesn't seem like the community has settled on a canonical form of dependency injection. Personally, I haven't found that using get_it to access my gql_client and pass that to my Query widget has been too burdensome.

I would rename the flutter library to FlutterQL so it stands out from all those gql_* packages 🤘 (as flutter is probably the most prominent use case for this library)

I've been thinking about renaming the package. I agree that flutter is likely the most common use case for this library, but I don't think we need to tie the package name to flutter (which may discourage other use cases).

from ferry.

Inline queries

I don't see any downsides since we are still backward compatible to separate .graphql files.
Regarding syntax highlighting, I know that VSCode extensions do support syntax highlighting and code-completion for gql... tags in JS/TS files, so it would be quite easy to make a pull request to those extensions to support @gql annotation with syntax highlighting and such.

Persisted queries

Yes it is an apollo-specific feature but I know that other server implementations adopted it since it is also supported in the relay client (that is not part of the apollo organization)
gqlgen to mention one is a golang graphql server with support for persisted queries.

Dependency injection

It is a common practice to include in a flutter library a provider widget and consumer widgets (flutter-graphql, flutter_bloc to mention two) and I think lots of beginners will appreciate having this supported in this library.
Having a canonical dependency injection in this package will also allow generated widgets like proposed in #3 to access the client convention-over-configuration.

Flutter vs pure dart

I think if this package includes flutter widgets and has the flutter SDK in its pubspec, it is a flutter library and there's no actual use case other than flutter right now. So I would go ahead and change the name. (ferry, flutter_ferry, flutterQL, whatever comes to your mind.) the current name is terribly similar to so many gql packages it's a pain :)
I'll comment further on the other issue.

from ferry.

Closing this issue. Feel free to open new issues with specific feedback.

from ferry.