Immutable type builders

[smkhalsa]

This PR adds support for building data, var, and schema types as built_value immutable data types.

Some improvements as a result of this change include:

1. equality checks for data objects
2. easy manual instantiation of data objects (useful when updating a normalized cache or mocking data).

A few notes:

Prefixes

built_value does not allow types to start with $ or start with a lowercase letter (without breaking serialization). Therefore, I've decided to prefix all generated types with "G" (which could stand for "Generated" or "GraphQL").

Serialization

built_value's serialization paradigm works by aggregating all serializers into a single global object. I've added an aggregating serializer_builder that collects all the serializers from each built class.

By design, built_value's serialization does not support multiple types with the same name since types are serialized by type name. Therefore, in order to avoid collisions, I've added a "Data" suffix to generated data types and a "Vars" suffix to generated variable types. Data and variable objects for "MyQuery" are therefore built as "GMyQueryData" and "GMyQueryVars", respectively.

code_builder limitations

The code_builder library does not include the ability to specify part directives and is missing some important operators. I've submitted PRs to fill these gaps, and in the meantime, I've added a dependency override on code_builder that points to my PR.

dart-lang/code_builder#277
dart-lang/code_builder#269

Edit: the above PRs have been merged.

req_builder

I've laid the groundwork for updating the req_builder to also use built_value. I don't think it's necessary to use built_value for op_builder or ast_builder. built_value already plays nice with objects that are immutable, and override "==" and "hashCode" (which DocumentNode and Operation already do). Instead, I've simply added a custom built_value serializer that serializes Operations by printing / parsing the DocumentNode.

However, since req_builder extends Request from gql_exec, there are two issues that I'd like feedback on:

1. The variables property on Request provides a Map<String, dynamic>, but ideally variables would reference the built vars object, allowing us to do something like the following:

final pokemonQuery = GPokemonQuery(
  (b) => b
    ..variables = GPokemonQueryVars(
      (b) => b
        ..first = 10
        ..offset = 0,
    ),
);

final morePokemonQuery = pokemonQuery.rebuild(
  (b) => b..variables.offset = 10,
);

We could save the built vars object to another property (e.g. builtVars) and implement the variables prop as a getter that runs builtVars.toJson(), but this could be confusing to the developer. Thoughts?

2. How should we handle serialization of Context on requests?

[smkhalsa]

I've updated gql_example_flutter to show the impact of these changes. NOTE: Until we resolve the issues with req_builder described above, we won't be able to actually run the example.

[micimize]

Doing variables typing right is tough without optional generic types. I think the best option is to just make variables a generic type (thus dynamic by default, for now). Seems the most forward looking, as eventually dart will let us make the type arg Map<String, dynamic>. We could provide some static method to construct the current default without a type arg also.

Another option is to rework the gql_exec Request inheritance chain, adding a base generic interface with something like an asExecutable getter for execution time. Feels awkward and invasive though.

The G prefix doesn't solve everything, but I think it's fine. I wonder how easily it'd be to an editor plugin that dims/styles the G.

This was a bit of an issue for me while trying to model union types: google/built_value.dart#831

[smkhalsa]

Doing variables typing right is tough without optional generic types. I think the best option is to just make variables a generic type (thus dynamic by default, for now). Seems the most forward looking, as eventually dart will let us make the type arg Map<String, dynamic>. We could provide some static method to construct the current default without a type arg also.

Another option is to rework the gql_exec Request inheritance chain, adding a base generic interface with something like an asExecutable getter for execution time. Feels awkward and invasive though.

Thanks for the feedback. I'll defer to @klavs here, but setting variables to dynamic for now seems to be an easy solution, even if it's not ideal.

The G prefix doesn't solve everything, but I think it's fine. I wonder how easily it'd be to an editor plugin that dims/styles the G.

Love the editor plugin idea. What are some cases that the G prefix doesn't solve? Maybe there's a better way to handle collisions?

[micimize]

What are some cases that the G prefix doesn't solve

Nevermind, I'm wrong. I was thinking query GMyQuery and query MyQuery {} would collide, but obviously not because they become GGMyQuery and GMyQuery

[smkhalsa]

What are some cases that the G prefix doesn't solve

Nevermind, I'm wrong. I was thinking query GMyQuery and query MyQuery {} would collide, but obviously not because they become GGMyQuery and GMyQuery

Yes, that's right

[klavs]

Sorry for my late response.

I've decided to prefix all generated types with "G"

I'm open to dropping the prefix and letting users use import prefixes if they choose so.

We could save the built vars object to another property (e.g. builtVars) and implement the variables prop as a getter that runs builtVars.toJson(), but this could be confusing to the developer. Thoughts?

Could you explain the potential confusion here?

How should we handle serialization of Context on requests?

Another hard one. Context may have both useful and useless information. (Or critical and nice-to-have information). I tend to think that Context should not be serializable because it kind-of contains the current state of request execution. It might be a fundamental design flaw in the Context if it's allowed to be used as a container for internal state and a container external request parameters. Maybe it should hidden from the users of a graphql client, by providing an alternative approach which lets the client decide what's serializable and what's not.

Doing variables typing right is tough without optional generic types. I think the best option is to just make variables a generic type (thus dynamic by default, for now).

I would love to avoid dynamic here. I believe in a layered approach where the existing Request is not type-aware, but some TypedRequest (non necessarily a sub-class of Request) is type-aware.
This way any request entering a link has a predictable, low-level shape. TypedRequest would live only until it reaches a link where it is materialized in a Request.

Again, sorry for the late response. I'll try to review the changes by the end of the week.

[smkhalsa]

Thanks @klavs

I'm open to dropping the prefix and letting users use import prefixes if they choose so.

We'll need to do some sort of transformation to bypass the lowercase letter limitation. If we simply capitalize all classes, we could have collisions since built_value's serialization requires unique class names.

Could you explain the potential confusion here?

If we have both a variables prop and a builtVars prop, it's not intuitively clear why they both exist and when each should be used.

Another hard one. Context may have both useful and useless information. (Or critical and nice-to-have information). I tend to think that Context should not be serializable because it kind-of contains the current state of request execution. It might be a fundamental design flaw in the Context if it's allowed to be used as a container for internal state and a container external request parameters. Maybe it should hidden from the users of a graphql client, by providing an alternative approach which lets the client decide what's serializable and what's not.

I'll think more about this. In ferry, since UpdateCacheHandlers are not called directly, we need a way to pass contextual data to the handlers (such as a user id) that may be necessary to update the cache but aren't available on the request (or response) itself. I decided to use the Context object since it was already available on the Request, and I see cache updates as a side effect of request execution.

However, the Context API is a bit clunky to interact with for this use case. It may be better to allow users to pass an arbitrary JSON map which they can access within the UpdateCacheHandler.

I would love to avoid dynamic here. I believe in a layered approach where the existing Request is not type-aware, but some TypedRequest (non necessarily a sub-class of Request) is type-aware.
This way any request entering a link has a predictable, low-level shape. TypedRequest would live only until it reaches a link where it is materialized in a Request.

That's fine. I think this is the right call. We can simply leave it to the client to convert the TypedRequest into a Request. However, this makes me wonder if we need req_builder (or op_builder) at all since clients will need to build their own TypedRequests anyway.

[klavs]

I think I would prefer not having the G prefix, but I see how it makes sense (considering __typename).

[smkhalsa]

I think this PR is in pretty good shape. We now have the following working:

1. use built_value for generated classes
2. add serializer_builder to aggregate built_value serializers
3. remove op builder (no longer necessary with changes to req_builder)
4. add custom scalar support
5. add support for graphql type overrides (necessary for custom scalars)
6. add support for custom serializers (necessary for custom scalars)
7. add JsonSerializer (makes it easier to write custom serializers with simply toJson / fromJson methods rather than having to use built_value's serializer format)
8. add and automatically apply OperationSerializer (uses JsonSerializer internally)

I've added a number of tests to the end_to_end_test package.

Custom scalars require some additional config in the build.yaml to work correctly:

1. type_overrides: a mapping of GraphQL types to arbitrary Dart types.
2. custom_serializers: a list of built_value serializers to include in the global Serializers object.

As an example, here's the build.yaml for end_to_end_test:

targets:
  $default:
    builders:
      gql_build|ast_builder:
        enabled: true
      gql_build|req_builder:
        enabled: true
        options:
          schema: end_to_end_test|lib/graphql/schema.graphql
      gql_build|serializer_builder:
        enabled: true
        options:
          schema: end_to_end_test|lib/graphql/schema.graphql
          custom_serializers:
            - import: 'package:end_to_end_test/date_serializer.dart'
              name: DateSerializer
      gql_build|schema_builder:
        enabled: true
        options:
          type_overrides:
            Date:
              name: DateTime
      gql_build|data_builder:
        enabled: true
        options:
          schema: end_to_end_test|lib/graphql/schema.graphql
          type_overrides:
            Date:
              name: DateTime
      gql_build|var_builder:
        enabled: true
        options:
          schema: end_to_end_test|lib/graphql/schema.graphql
          type_overrides:
            Date:
              name: DateTime

As you can see, we are repeating the type_overrides map in schema_builder, data_builder, and var_builder. Ideally, we'd have a way to share common config across builders, but I haven't found a way to do so, and this works for now.

@klavs @micimize Thanks for your feedback. Please let me know if there's anything else before this gets merged.

[smkhalsa]

It seems the CI is failing due to inconsistencies in import order in the generated files between builds.

@klavs do we need the git diff check?

[smkhalsa]

I'm going to go ahead and merge this since I haven't heard back in a while.