Using fragments prevents using cache

[baconcheese113]

I'm trying to create a sample app for others that want to use graphql-flutter and artemis, while learning how to use them myself.
It appears that fragments can only be used with FetchPolicy.noCache (not even FetchPolicy.networkOnly. Removing the cache eliminates a huge part of what makes graphl-flutter appealing, so is there any way to use artemis with fragments and cache?

In main.dart

  await initHiveForFlutter();

In app.dart

return GraphQLProvider(
      client: ValueNotifier(GraphQLClient(
        cache: GraphQLCache(store: HiveStore()),
        link: HttpLink("https://graphql-pokeapi.graphcdn.app"),
      )),
      child: MaterialApp(
        title: "Pokemon Demo",
        onGenerateRoute: (settings) {
          switch (settings.name) {
            case '/':
            default:
              return MaterialPageRoute(builder: (_) => PokemonListScreen());
          }
        },
      ),
    );

In pokemon_list.query.graphql

query PokemonList {
    pokemons(limit: 50) {
        count
        results {
            id
            ...pokemonListCard_item
        }
    }
}

In pokemon_list_card.fragment.graphql

fragment pokemonListCard_item on PokemonItem {
    url
    name
    image
    artwork
}

In pokemon_list.dart

return Query(
        options: QueryOptions(
          document: POKEMON_LIST_QUERY_DOCUMENT,
          operationName: POKEMON_LIST_QUERY_DOCUMENT_OPERATION_NAME,
          fetchPolicy: FetchPolicy.noCache, // This prevents using the cache!!
        ),
        builder: (QueryResult result, {fetchMore, refetch}) {
          if (result.isLoading) return CircularProgressIndicator();
          if (result.hasException) return Center(child: Text(result.exception!.toString()));

          print("result is $result");
          // >>>>>result.data is missing all fragment data with a different fetch policy <<<<<
          final data = PokemonList$Query.fromJson(result.data!);

          final cardList = data.pokemons!.results!.map((pokemon) {
            print("Pokemon name: ${pokemon!.name}");
            return PokemonListCard(itemFrag: pokemon);
          }).toList();

          return RefreshIndicator(
              child: ListView(children: cardList),
              onRefresh: () async {
                await refetch!();
              });
        });

[baconcheese113]

You can find the example project that replicates this problem here https://github.com/baconcheese113/graphql-flutter-artemis-example

[baconcheese113]

#350

[baconcheese113]

Okay...12 hours straight debugging this problem 🎉

Long Explanation

While breakpoint debugging the internals of graphql-flutter, normalize, and Hive I noticed that query_manager.dart#L255 attemptCacheWriteFromResponse() fails to write the query to the cache with the fragmentNodes.

The strange part is that mapFetchResultToQueryResult() maps response with the fragment information correctly. But since attemptCacheWriteFromResponse() returns true, it overwrites queryResult a few lines later on line 279

Eventually I dug all the way to normalizes normalizeNode() function used to resolve attemptCacheWriteFromResponse() and noticed that dataForNode is passed in with the correct fragment data, but it is stripped out before the deepMerge() in the same function.

At this point I decided to skim through the normalize README and this caught my attention

The normalize function only normalizes entities that include a __typename field and a valid ID.

So I added the id field to my fragment and everything worked perfectly. I also tried removing the id field and replacing with __typename but that didn't work. I then tried removing id from the results field in the main query and that removed the entire results field from the returned data.

TLDR

Every field that returns a list of objects must include the id field in order to be stored in the cache properly, and fragments need __typename. If stored incorrectly it will mess up the data returned from the network as well.

Prevention

I'm not sure how difficult this would be, but if there was a way to add id to every requested field that has an id the same way __typename is added then nobody would run into this error in the future. I believe this is what Relay does and why users don't necessarily need to add __typename and id everywhere, but I could be mistaken. I imagine there are a lot of bugs in current Artemis projects that haven't been caught due to this issue

[vasilich6107]

Hi @baconcheese113
Thanks for your time investment in this issue.
GraphQLCache has dataIdFromObject function which could help you to fix normalization issues.

We cannot add id by default to all items cause it is not a standard name.

[baconcheese113]

I'm working on a PR now to add the user specified typeNameField to all ClassDefinition's. Something like this:
class_definition.dart#L36

/// Instantiate a class definition.
ClassDefinition({
  required Name name,
  Iterable<ClassProperty> properties = const [],
  this.extension,
  this.implementations = const [],
  this.mixins = const [],
  this.factoryPossibilities = const {},
  ClassPropertyName? typeNameField,
  this.isInput = false,
})  : typeNameField = typeNameField ?? ClassPropertyName(name: '__typename'),
      properties = properties.any((p) => p.name.name == (typeNameField?.name ?? '__typename'))
          ? properties
          : [
              ...properties,
              ClassProperty(
                type: TypeName(name: 'String'),
                name: typeNameField ?? ClassPropertyName(name: '__typename'),
                annotations: ['JsonKey(name: \'${typeNameField?.name ?? '__typename'}\')'],
                isResolveType: true,
              )
            ],
      super(name: name);

This would at least align the functionality with the functionality of gql that a lot of users would be migrating to Artemis from.

But couldn't we also read the schemaMap while creating a new ClassDefinition and add id if it exists on the schema? If not everyone uses the id field it could be customizable the same way typeNameField is

[vasilich6107]

@baconcheese113 we already have fix for that
#254

[baconcheese113]

Any plans to release a new stable version? I'm not sure if all users are aware there are large differences between the stable and beta version that they need to read a different readme to discover, I wasn't