[gql_code_builder ] bugfix for reuse-fragments feature #417

dehypnosis · 2023-09-01T13:26:57Z

Hi @knaeckeKami
This PR is a succession of #413
After the above PR, our team have developed our ongoing project with dev release version which includes above PR.
And we made a small improvement and a bugfix to make the reuse-fragment feature stable.
So here is a PR to finalize this feature.

Changes

Fix inline fragments data class serialization runtime error with ferry_generator:graphql_builder.data_class_config.reuse_fragments option enabled.
Fix WhenExtension to not to create redundant mappers for interfaces which doesn't make sense because interfaces don't have __typename. (But for the backward compatibility, this change is only applied when reuse_ragment option is enabled.)

…e-fragments. - Fix reuse-fragments for inline fragments serialization runtime error. - Fix reuse-fragments for inline fragments: - Below query will consider CommunityContent interface while deciding reuse-fragment of CommunityFeed which implements CommunityContent. ```graphql node: { ... on CommunityContent { liked viewed } ... on CommunityFeed { ...CommunityFeedFragment } ... on CommunityReply { ...CommunityReplyFragment } } ``` - Additionally fix WhenExtension to not to create redundant mappers for interfaces which doesn't make sense because interfaces don't have __typename. (cherry picked from commit 7206742)

…not-existing fragment used

knaeckeKami · 2023-09-01T13:41:59Z

Thanks, I'll have a proper look later.

One thing that i noticed on the first glance, you seem to have implemented another way of calculating the possibleTypes map.

Would it be feasible to leverage https://github.com/gql-dart/gql/blob/master/codegen/gql_code_builder/lib/src/utils/possible_types.dart instead ?

codegen/gql_code_builder/lib/data.dart

knaeckeKami · 2023-10-07T22:22:23Z

sorry for the late response!
please pull the latest changes from master to ci succeeds

…ts-inline

dehypnosis · 2023-10-11T17:43:46Z

@knaeckeKami how are you doing well? just merged upstream/master branch.

knaeckeKami · 2023-10-11T22:19:25Z

Thanks! It seems there are now difference in the generated code in end_to_end_test, despite the reuse_fragments flag being disabled.
I'll look into why this is.

dehypnosis · 2023-10-13T01:07:37Z

@knaeckeKami oh, sorry for unexpected trouble. Is that related to WhenExtension ?

knaeckeKami · 2023-10-13T09:26:36Z

Not sure yet, didn't have a proper look, but you can check the diff in the CI output: https://github.com/gql-dart/gql/actions/runs/6485343593/job/17611211975?pr=417

dehypnosis · 2023-10-13T13:54:04Z

codegen/gql_code_builder/lib/src/inline_fragment_classes.dart

+        superclassSelections: {
+          name: SourceSelections(url: null, selections: selections),
+          ...Map.fromEntries(superclassSelections.entries.map((e) => MapEntry(
+              "${e.key}__as${inlineFragment.typeCondition!.on.name.value}",
+              e.value))),
+        },


@knaeckeKami I think the cause is that this fix about buildInlineFragmentClasses is not checking the feature flag.
This fix is made to let ..FragmentData classes cover more interfaces like by adding GheroFieldsFragment__asHuman interface to GheroFieldsFragmentData__asHuman. And I think changes made by this are all additions for missing interfaces but no regressions of existing interfaces. So there would be no regression issue even without the feature flag.

I just run the code generation without the flag and unfortunately, it produces code that does not compile.

the following graphql document caues problems:

query HeroWithInterfaceSubTypedFragments($episode: Episode!) { hero(episode: $episode) { ...heroFieldsFragment } } fragment heroFieldsFragment on Character { id name ...on Human { ...humanFieldsFragment } ...on Droid { ...droidFieldsFragment } } fragment humanFieldsFragment on Human { homePlanet friends { ...on Droid { id name ...droidFieldsFragment } ...on Human { id name homePlanet } } } fragment droidFieldsFragment on Droid { primaryFunction }

this adds another superclass to GheroFieldsFragmentData__asHuman which conflicts with the implementation:

'GheroFieldsFragmentData__asHuman.friends' ('BuiltList<GheroFieldsFragmentData__asHuman_friends?>? Function()') isn't a valid override of 'GheroFieldsFragment__asHuman.friends' ('BuiltList<GheroFieldsFragment__asHuman_friends?>? Function()'). (Documentation) The member being overridden (hero_with_interface_subtyped_fragments.data.gql.dart:151).

@knaeckeKami sorry to bother you, I will turn off new logics when there is no feature flag. Will share again soon.

no worries! i really appreciate you working on this! :)

@knaeckeKami
How are you doing well? Finally came back here.
Sorry for late response. Just made the update. 😀

codegen/gql_code_builder/lib/src/when_extension.dart

…not on

knaeckeKami · 2023-12-15T22:40:46Z

Thanks!

A user reported some issues with unions with a lot of possible values, see here:

https://github.com/LiLatee/flutter_ferry_unions_issue

I noticed that the DFS searches in all permutations of the given union types, which leads to exponential code generation time .

This is a possible fix:

https://github.com/gql-dart/gql/tree/possible_reuse_fragment_fix

Please add a test that checks if this bug is fixed by this PR

LiLatee · 2023-12-21T21:00:26Z

Hi @knaeckeKami @dehypnosis! 😄

I've prepared a repo with an example that generates a code with errors. In short, overridden fields have a wrong return type. I think that the problem exists because I have two different fragments defined on a single type defined in schema. In my case I have two fragments::
ActivityB and ActivityBWithoutSharedItem defined on type ActivityB.

Is there a quick solution for that? 🤔

LiLatee · 2024-01-01T16:09:17Z

I've spent a few more hours and created an even much simpler example to reproduce the issue and I really cannot understand what's the problem here.

I've found three solutions to fix the issue by modifying fragments:

Remove import. Just import (even without using it) causes the error.

Remove one fragment and use its content directly. Removing ItemEdge here and pasting that fragment's content fixes the issue

Or removing one union field activityData

Each of these modifications fixes the issue and I need to use just one of them.

Is there a chance that you could take a look at that in the near future? 🙏

knaeckeKami · 2024-01-01T16:36:33Z

Hi!

I also looked a bit into it.
I think the issue even occurs in the end_to_end_test in this query:

hero_with_interface_subtyped_fragments.graphql.

query HeroWithInterfaceSubTypedFragments($episode: Episode!) {
  hero(episode: $episode) {
    ...heroFieldsFragment
  }
}

fragment heroFieldsFragment on Character {
  id
  name

  ...on Human {
    ...humanFieldsFragment
  }

  ...on Droid {
    ...droidFieldsFragment
  }
}

fragment humanFieldsFragment on Human {
  homePlanet
  friends {
    ...on Droid {
      id
      name
      ...droidFieldsFragment
    }
    ...on Human {
      id
      name
      homePlanet
    }
  }
}

fragment droidFieldsFragment on Droid {
  primaryFunction
}

And it seems to be triggered by Inline fragment spreads with typeconditon where the selection is just one fragment spread.

But I don't know much more yet.

knaeckeKami · 2024-01-05T23:52:49Z

I now understand what the issue is.

I found a way to fix it in gql_code_builder, but not sure if this breaks smth else.

@LiLatee can you try

dependency_overrides:
 gql_code_builder:
    git:
        url: https://github.com/gql-dart/gql/tree/repro_reuse_fragments
        ref: repro_reuse_fragments
        path: codegen/gql_code_builder

and give me feedback?

LiLatee · 2024-01-06T13:38:48Z

@knaeckeKami
Unfortunately still the same issue on the example repository 😞

btw I guess that's a proper dependency overrides

  gql_code_builder:
    git:
      ref: repro_reuse_fragments
      url: https://github.com/gql-dart/gql.git
      path: codegen/gql_code_builder

knaeckeKami · 2024-01-06T16:12:34Z

yes, thanks for fixing the broken link.

Sorry. I did not push all the code. please try again.

LiLatee · 2024-01-06T17:19:57Z

I am getting other errors right now

I can add Data to the name, but not sure if it's fine.

I don't have time right now, but I can check it more deeply tomorrow

knaeckeKami · 2024-01-06T17:52:24Z

Ok, I had a suspicion that it would be not that easy ;)

Would appreciate if you could create failing reproduction to the end_to_end_test repo.

But I know now what the issue is, and I think I can find a fix in the near future.

LiLatee · 2024-01-06T18:14:08Z

Would appreciate if you could create failing reproduction to the end_to_end_test repo.

Sure, I can try to take care of it tomorrow 👌

But I know now what the issue is, it's only and I think I can find a fix in the near future.

Would be awesome 🤞

knaeckeKami

Commented on the code I think is the culprit

knaeckeKami · 2024-01-07T06:27:23Z

codegen/gql_code_builder/lib/src/inline_fragment_classes.dart

+        typeOverrides: typeOverrides,
+        superclassSelections: {
+          name: SourceSelections(url: null, selections: selections),
+          if (dataClassAliasMap.isNotEmpty)


I think this condition is not correct.
Inline Fragments can add new fields in selections which can cause conflicts in extending the fragments.

It also causes the build error in the end to end test package.

When Removing this, it works again

LiLatee · 2024-01-07T12:16:35Z

Would appreciate if you could create failing reproduction to the end_to_end_test repo.

@knaeckeKami should I create a failing case for the code before you commented that condition above?
Because your comment removes some part of fragment reusing. E.g. before that change my user.data.gql.dart file had 212 lines and now it has 712, because new fragments (that already exist) are created again.

knaeckeKami · 2024-01-07T13:25:59Z

Failing in the sense that it's more code or that the code doesn't compile?

Anyway, yes please share.

I know that this might increase the code size but this optimization is not correct in the way it is now. But maybe we can find I way to make it work again.

dehypnosis · 2024-01-08T04:27:25Z

Sorry guys, I'm busy doing my day works nowadays. I will dive into to take a look in this weekend if the issue is not resolved until then.

LiLatee · 2024-01-08T07:35:02Z

@knaeckeKami oh I've finally found one issue. It looks like the order of the imports in .graphql files is important.
e.g.
Case 1:

Case 2:

This example uses repro_reuse_fragments version (commit 6ae9dfa)

knaeckeKami · 2024-01-08T08:59:30Z

thanks!

@LiLatee implemented another approach to reuse fragment classes in #437 . this is just an experiment at this point, but feel free to try it out.

dehypnosis added 4 commits August 14, 2023 22:52

make whenExtension modification backward-compatible, show error when …

c3b1a5d

…not-existing fragment used

fix superclassSelections of inlineFragment data classes

c047ece

lint: prefer_final_in_for_each

17e906b

dehypnosis commented Sep 2, 2023

View reviewed changes

codegen/gql_code_builder/lib/data.dart Show resolved Hide resolved

dehypnosis added 2 commits September 28, 2023 10:22

fix already aliased fragment to not stop creating deeper nested alias

122da12

enhance aliasing by resuing deeply nested fragments inside fragments too

d3497bf

Merge remote-tracking branch 'upstream/master' into fix/reuse-fragmen…

c3a5ce9

…ts-inline

dehypnosis commented Oct 13, 2023

View reviewed changes

codegen/gql_code_builder/lib/src/when_extension.dart Show resolved Hide resolved

fix(reuse-fragment): turn off super class expanding when the flag is …

253d933

…not on

knaeckeKami requested changes Jan 7, 2024

View reviewed changes

knaeckeKami mentioned this pull request Jan 7, 2024

reuse fragments by their selection set #437

Draft

LiLatee mentioned this pull request Jan 8, 2024

(fix/reuse-fragments) New test case for issue with implementing not existing classes #438

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gql_code_builder ] bugfix for reuse-fragments feature #417

[gql_code_builder ] bugfix for reuse-fragments feature #417

dehypnosis commented Sep 1, 2023 •

edited

Loading

knaeckeKami commented Sep 1, 2023

knaeckeKami commented Oct 7, 2023

dehypnosis commented Oct 11, 2023

knaeckeKami commented Oct 11, 2023

dehypnosis commented Oct 13, 2023

knaeckeKami commented Oct 13, 2023

dehypnosis Oct 13, 2023 •

edited

Loading

knaeckeKami Oct 15, 2023

dehypnosis Oct 18, 2023

knaeckeKami Oct 20, 2023

dehypnosis Nov 13, 2023

knaeckeKami commented Dec 15, 2023

LiLatee commented Dec 21, 2023

LiLatee commented Jan 1, 2024

knaeckeKami commented Jan 1, 2024

knaeckeKami commented Jan 5, 2024 •

edited

Loading

LiLatee commented Jan 6, 2024 •

edited

Loading

knaeckeKami commented Jan 6, 2024

LiLatee commented Jan 6, 2024 •

edited

Loading

knaeckeKami commented Jan 6, 2024 •

edited

Loading

LiLatee commented Jan 6, 2024

knaeckeKami left a comment •

edited

Loading

knaeckeKami Jan 7, 2024

LiLatee commented Jan 7, 2024

knaeckeKami commented Jan 7, 2024

dehypnosis commented Jan 8, 2024

LiLatee commented Jan 8, 2024 •

edited

Loading

knaeckeKami commented Jan 8, 2024

[gql_code_builder ] bugfix for reuse-fragments feature #417

Are you sure you want to change the base?

[gql_code_builder ] bugfix for reuse-fragments feature #417

Conversation

dehypnosis commented Sep 1, 2023 • edited Loading

Changes

knaeckeKami commented Sep 1, 2023

knaeckeKami commented Oct 7, 2023

dehypnosis commented Oct 11, 2023

knaeckeKami commented Oct 11, 2023

dehypnosis commented Oct 13, 2023

knaeckeKami commented Oct 13, 2023

dehypnosis Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

knaeckeKami Oct 15, 2023

Choose a reason for hiding this comment

dehypnosis Oct 18, 2023

Choose a reason for hiding this comment

knaeckeKami Oct 20, 2023

Choose a reason for hiding this comment

dehypnosis Nov 13, 2023

Choose a reason for hiding this comment

knaeckeKami commented Dec 15, 2023

LiLatee commented Dec 21, 2023

LiLatee commented Jan 1, 2024

knaeckeKami commented Jan 1, 2024

knaeckeKami commented Jan 5, 2024 • edited Loading

LiLatee commented Jan 6, 2024 • edited Loading

knaeckeKami commented Jan 6, 2024

LiLatee commented Jan 6, 2024 • edited Loading

knaeckeKami commented Jan 6, 2024 • edited Loading

LiLatee commented Jan 6, 2024

knaeckeKami left a comment • edited Loading

Choose a reason for hiding this comment

knaeckeKami Jan 7, 2024

Choose a reason for hiding this comment

LiLatee commented Jan 7, 2024

knaeckeKami commented Jan 7, 2024

dehypnosis commented Jan 8, 2024

LiLatee commented Jan 8, 2024 • edited Loading

knaeckeKami commented Jan 8, 2024

dehypnosis commented Sep 1, 2023 •

edited

Loading

dehypnosis Oct 13, 2023 •

edited

Loading

knaeckeKami commented Jan 5, 2024 •

edited

Loading

LiLatee commented Jan 6, 2024 •

edited

Loading

LiLatee commented Jan 6, 2024 •

edited

Loading

knaeckeKami commented Jan 6, 2024 •

edited

Loading

knaeckeKami left a comment •

edited

Loading

LiLatee commented Jan 8, 2024 •

edited

Loading