comigor / artemis

Build dart types from GraphQL schemas and queries
MIT License
495 stars 119 forks source link

Lazy Getters #14

Open lucasavila00 opened 5 years ago

lucasavila00 commented 5 years ago

The way graphql_flutter implements NormalizedCache would require denormalizing data on every build call, making allocations for everything under that type for fields that may not be even relevant.

Like a list of friends and respective chat history that would require denormalizing the chat history and allocate it event if I just asked for the friends names.

I would suggest using lazy getters that use their public Map<String,dynamic> interface to interact with data.

That means type generation becomes "cost free" as the compiler would remove most of the indirection it generates and wouldn't require any allocation, dereferencing, etc.

The way I'm currently dealing with it is like this:

  # addressAutosuggest: [AddressAutosuggestResponse!]!
  addressAutosuggest {
    locationName # String!
    latitude # Float!
    longitude # Float!
  }
// generated

class AddressAutosuggestAddressAutosuggestResponse {
  AddressAutosuggestAddressAutosuggestResponse(this._d);

  final Map<String, dynamic> _d;

  String get locationName => this._d["locationName"];
  double get latitude => this._d["latitude"].toDouble();
  double get longitude => this._d["longitude"].toDouble();

}

class AddressAutosuggest {
  AddressAutosuggest(this._d);

  final Map<String, dynamic> _d;

  List<AddressAutosuggestAddressAutosuggestResponse> get addressAutosuggest =>
      (this._d["addressAutosuggest"] as List)
          .map<AddressAutosuggestAddressAutosuggestResponse>(
              (dynamic addressAutosuggestResponse) =>
                  AddressAutosuggestAddressAutosuggestResponse(
                      addressAutosuggestResponse))
          .toList();

}
comigor commented 5 years ago

Hello again @lucasavila00, That makes sense. It's not a priority, though, given this would require redoing most of the work json_serializable does.

It makes more sense to have json_serializable (or an extension to it, or another generator like it), to support this kind of lazy read, as other projects could benefit from it, instead of recreating it here.

What do you think about requesting this behavior on json_serializable, or even starting another library to do it?

lucasavila00 commented 5 years ago

I don't think there is really a need to support more than this specific use case. Graphql-flutter may change their implementation details and then I wouldn't see a reason to mantain this bigger scoped project.

Graphql-flutter is being used in production and works fine and I don't see why Artemis couldn't support it first class.

@comigor As I believe you like Apollo API design and graphql-flutter tries to immitate it as closely as possible there isn't a conflict here.

Personally I wouldn't be interested in mantaining another package just so Artemis works for me but would be interested in helping Artemis work with my current setup.

Is there a reason not to support graphql_flutter first class?

comigor commented 5 years ago

Sure, but first-class support is different from coupling everything in one library to make another one work. Lazy fields shouldn't be Artemis responsability, but rather something it could get from json_serializable out of the box. Also, keep in mind the goal of Artemis is to be agnostic of environment (it should work with pure Dart, without graphql_flutter).

This lazy feature is clearly some enhancement of the way json_serializable generated classes deal with data and, as I said, other projects could benefit from this laziness. That's why it seems better to have it as an option of json_serializable or as another library/generator.

If we implement this behavior on Artemis, we'd need to basicaly stop using json_serializable and write the custom class with lazy fields, is that so? Or do you have another way of doing this in mind?

lucasavila00 commented 5 years ago

In my mind I'd put the lazy getters behind a flag and almost "fork" json_serializable for our specific needs. This would allow faster development under a smaller test suite for this specific use case.

What I hadn't considered is upstreaming the feature to json_serializable. I'll investigate if this is possible and if the project is willing to accept this addition.