Skip to content
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

[gql_build] ability to define scalar map in build.yaml #61

Closed
vanelizarov opened this issue Feb 21, 2020 · 7 comments
Closed

[gql_build] ability to define scalar map in build.yaml #61

vanelizarov opened this issue Feb 21, 2020 · 7 comments

Comments

@vanelizarov
Copy link

vanelizarov commented Feb 21, 2020

Let's say I have custom scalars in my schema, e.g. Email which actually is String. It's okay that builder generates wrapper class for it. But there are cases when i need that mapping, e.g. with Uint or Time . Builder produces a wrapper class which stores value in String variable, but it's not okay because when I send such variable within query to server, exception occurs. So, the good option to have would be ability to define scalar map in build.yaml like in artemis

@klavs
Copy link
Contributor

klavs commented Feb 22, 2020

@vanelizarov is the underlying issue that some scalars may not be strings (over JSON)?

@vanelizarov
Copy link
Author

vanelizarov commented Feb 22, 2020

@klavs yeah, that's the case. For example Uint which is actually int in Dart. It comes as number from server, but I cannot simply print() such property because of typecasting exception, of course other operations with these properties are unavailable for the same reason
Also it would be cool to map other scalars like Email (which is actually a plain string and comes from server as string) to Dart's String just for optimization sake 🙂

@klavs
Copy link
Contributor

klavs commented Feb 22, 2020

Also it would be cool to map other scalars like Email (which is actually a plain string and comes from server as string) to Dart's String just for optimization sake

It could be added as an option, but I do not think it's good to optimize for performance here. All the custom scalars are wrapped to avoid unintentional assignments. If email: Email is treated as a String, developer is able to assign any String to email without any static warning. Wrapped types force developer to explicitly cast primitive types to scalars. I'm actually considering adding a wrapper to ID type, because ids are supposed to be opaque.

For example Uint which is actually int in Dart. It comes as number from server, but I cannot simply print() such property because of typecasting exception, of course other operations with these properties are unavailable for the same reason

Yes, such functionality should probably be added. For the reasons explained above, I think it would be first implemented as a map of custom scalar to JSON-compatible dart type (int, double, String, bool).

@vanelizarov
Copy link
Author

It could be added as an option, but I do not think it's good to optimize for performance here. All the custom scalars are wrapped to avoid unintentional assignments. If email: Email is treated as a String, developer is able to assign any String to email without any static warning. Wrapped types force developer to explicitly cast primitive types to scalars. I'm actually considering adding a wrapper to ID type, because ids are supposed to be opaque.

Oh, now I understand, thanks for the explanation!

Yes, such functionality should probably be added. For the reasons explained above, I think it would be first implemented as a map of custom scalar to JSON-compatible dart type (int, double, String, bool).

So, can you please point me where to start working on such option? I'm interested in implementing it and making PR in case of success 🙂
All I've found for now is defaultTypeMap constant here and that it gets passed through typeRef() to code builder operations, but I cannot understand, how to pass down custom map from build.yaml and how to do it for all builders at once without copy-pasting this map for each builder

@klavs
Copy link
Contributor

klavs commented Feb 22, 2020

It's going to be affecting many files. gql_build would be responsible of reading in the custom map and passing it to gq_code_builder. Every builder interested in schema types would receive the custom map and pass it down every function call till the accessor is reached.

Unfortunately, it would most likely not be implementable once without copy-pasting.

As you noticed, defaultTypeMap and typeRef were initial attempt to solve this problem, but I lowered it's priority and did not finish it.

@vanelizarov
Copy link
Author

vanelizarov commented Feb 27, 2020

@klavs ok, I understand what you mean, thanks!

Finally I ended up with temp solution by copying gql_build and gql_code_builder to my project and changing some sources:

  1. Defined customScalarMap constant in gql_code_builder/lib/src/common.dart
const defaultTypeMap = <String, Reference>{
  // ...
};

const customTypeMap = <String, Reference>{
  "Uint": Reference("int"),
  "Time": Reference("String"),
  "Upload": Reference("File", "dart:io"),
  "Email": Reference("String"),
  // ...
};
  1. Added condition to check if current typeName exists in customTypeMap to gql_code_builder/lib/src/operation/data.dart for further checks to not add .value in getters/setters:
Method _buildGetter(
  DocumentNode schema,
  String schemaUrl,
  SelectionNode node,
  String prefix,
  String type,
) {
  if (node is FieldNode) {
    // ...
    final typeName = unwrappedTypeNode.name.value;

    TypeDefinitionNode fieldTypeDef;
    if (!customTypeMap.containsKey(typeName)) {
      fieldTypeDef = _getTypeDefinitionNode(
        schema,
        typeName,
      );
    }

    final typeMap = {
      ...defaultTypeMap,
      ...customTypeMap, // also added this one
      if (node.selectionSet != null)
        typeName: refer("$prefix\$$name")
      else if (fieldTypeDef != null)
        typeName: refer(typeName, schemaUrl)
    };
    // ...
}

Similarly did it for gql_code_builder/lib/src/operation/var.dart

Method _buildSetter(
  VariableDefinitionNode node,
  DocumentNode schema,
  String schemaUrl,
) {
  final unwrappedTypeNode = _unwrapTypeNode(node.type);
  final typeName = unwrappedTypeNode.name.value;

  TypeDefinitionNode argTypeDef;
  if (!customTypeMap.containsKey(typeName)) {
    argTypeDef = _getTypeDefinitionNode(
      schema,
      typeName,
    );
  }

  final typeMap = {
    ...defaultTypeMap,
    ...customTypeMap, // also added this one
    if (argTypeDef != null) typeName: refer(typeName, schemaUrl),
  };
  // ...
}

And for gql_code_builder/lib/src/schema/input.dart:

Method _buildSetter(
  InputValueDefinitionNode node,
  DocumentNode doc,
) {
  final unwrappedTypeNode = _unwrapTypeNode(node.type);
  final typeName = unwrappedTypeNode.name.value;

  TypeDefinitionNode argTypeDef;
  if (!customTypeMap.containsKey(typeName)) {
    argTypeDef = _getTypeDefinitionNode(
      doc,
      typeName,
    );
  }

  final typeMap = {
    ...defaultTypeMap,
    ...customTypeMap, // also added this one
    if (argTypeDef != null) typeName: refer(typeName),
  };
  // ...
}
  1. Changed pubspec.yaml imports in locally copied gql_build:
dependencies:
  # ...
  gql_code_builder:
    path: ../gql_code_builder

And in my project:

dev_dependencies:
  # ...
  gql_build:
    path: ../gql_build

I know that this solution doesn't quite correlate with what you wrote, but it works for me. Hope sometimes you will release "native" version of it 🙂

@smkhalsa
Copy link
Member

@vanelizarov initial custom scalar support has been added, although docs still need to be updated.

You will need to provide a "type_overrides" yaml map in your build.yaml. Please note that you need to duplicate the type_overrides map for each builder that depends on the schema in your build.yaml.

      gql_build|data_builder:
        enabled: true
        options:
          schema: pokemon_explorer|lib/schema.graphql
          type_overrides:
            MyScalar:
              name: MyScalar
              import: "path/to/package"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants