Add back mergeInFieldsFromFragmentSpreads option

[tahirmt]

Adding back the option to merge in fields from fragment spreads. This configuration option will allow a smaller code size for people who have a huge number of fragments.

This PR is still a draft because I want to add tests first but creating a PR for early review.

I know this feature has been planned for 1.3 but I still wanted to see if I can help speed up some parts of it.

Closes #2560

[apollo-cla]

@tahirmt: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[netlify]

👷 Deploy request for apollo-ios-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	c521307

[tahirmt]

Doesn’t look like there are any existing tests for this. If I set the default to false nothing fails. I might need a little guidance on how best to add tests.

[AnthonyMDev]

Thanks so much for this @tahirmt! I need to give this a bit of thought, but this looks like a good start! Let me see if I can do some research and I'll come back to you with some more information on what needs to happen to get this merged soon!

[tahirmt]

@AnthonyMDev I added some tests! My changes will not improve code generation time if the option is turned off but it will allow the option to reduce generated code size.

[AnthonyMDev]

Just wanted to give you an update on this. I've been looking into it, and its a bit more involved than this PR to make this work properly (lots of edge cases!) But I'm working on coming up with a plan to get this merged as soon as I can!

[tahirmt]

Thanks for the update! Happy to help if I can.

[AnthonyMDev]

After discussing this a bit more with @calvincestari, we have come up with an approach! Removing all of the merged selections is a bit heavy-handed, as it also removes merging of parent fields into child inline fragments, which are usually used for type-cases.

The most requested use of this feature is to stop merging selections from named fragments, as those are often isolated to be used after converting to the specific fragment definition. But not merging inline fragment fields would make both operations and generated named fragments more difficult to work with.

Additionally, we still want to continue to merge the fragment accessors (that is, the generated properties that let you convert your model into a fragment model) across child inline fragments as well.

We plan for the initial implementation of this feature to only remove the merging of selections from named fragments. If there are still requests from the community to disable more fragment merging, then we can consider adding an option to stop merging of inline fragments. But we anticipate there to be greater complications there with some edge cases, as inline fragments are useful for checking type cases as well as when using the @include/skip directives.

Specification

We would like to anticipate the addition of other options in the future here, so we want to turn mergeInFieldsFromFragmentSpreads into an enum instead of a Bool and rename it to FragmentMergingStrategy.

Here is what we have in mind:

Add a new property to OutputOptions:

public let fragmentMergingStrategy: FragmentMergingStrategy

The FragmentMergingStrategy can be defined as:

public enum FragmentMergingStrategy {
  /// The default value. 
  /// Merges selections included from all fragments into the `SelectionSet` they are spread into.
  case mergeAllFragmentSpreads

  /// Merges selections included from all **inline** fragments 
  /// into the `SelectionSet` they are spread into.
  /// Does not merge selections included from **named** fragment spreads 
  /// into the `SelectionSet` they are spread into.
  /// This still merges fragment accessors across child inline fragments.
  case excludeNamedFragmentSpreads
}

The default value for fragmentMergingStrategy should be .mergeAllFragmentSpreads.

---

In the future, additional values can be added to the FragmentMergingStrategy enum if deemed necessary like so:

public enum FragmentMergingStrategy {
  /// The default value. 
  /// Merges selections included from all fragments into the `SelectionSet` they are spread into.
  case mergeAllFragmentSpreads

  /// Merges selections included from all **inline** fragments 
  /// into the `SelectionSet` they are spread into.
  /// Does not merge selections included from **named** fragment spreads 
  /// into the `SelectionSet` they are spread into.
  /// This still merges fragment accessors across child inline fragments.
  case excludeNamedFragmentSpreads

  /// Does not merge selections included from **named or inline** fragment spreads 
  /// into the `SelectionSet` they are spread into.
  /// This still merges fragment accessors across child inline fragments.
  case excludeAllFragmentSpreads

  /// Does not merge selections included from **named or inline** fragment spreads 
  /// into the `SelectionSet` they are spread into.
  /// Does not merges fragment accessors across child inline fragments.
  ///
  /// This generates completely flat models that reflect the shape of your operation/fragment definitions directly.
  case mergeNone
}

---

This is a bit more involved than the implementation you've got in this PR @tahirmt. If you would like to take a crack at implementing this, I'm more than happy to guide you through it a bit, review your code, and get this merged in! Starting to look at how I would solve this, there are a couple of significant issues that I'm not sure how I'd solve without a bit more time and thought.

It's going to involve altering how the IR layer either exposes the merged selections or how it computes them in the first place. And there are a lot of complex moving parts there.

[tahirmt]

Time permitting I can take a look at this. I will need to understand how it works a bit better first though. Ideally we shouldn't compute the fields if they won't be used so changing it at that level is what's best. I have never gone that in depth in Apollo's code before so it might take me a bit.

[AnthonyMDev]

Yeah, it's quite a lot to sort through! If you don't get an opportunity to work on this, it is on my roadmap and I plan on fixing this soon!

[calvincestari]

I changed this PR to be a draft until we're ready for review. Prevents any accidental merging.

[pwillsey]

Is there any update on when we can expect this change to be merged ? We've experienced a very large increase in app size since we updated from 0.53 to 1.0.6

[AnthonyMDev]

We are working to get there! This is really not a straight forward problem to solve, but we do intend on solving it. We're working through our Release 1.1 issues right now. And this is currently the only issue slated for the 1.2 Release which will come directly after that.

Thanks for your patience while we sort through this stuff, lots of tough work to be done!

[pwillsey]

We are working to get there! This is really not a straight forward problem to solve, but we do intend on solving it. We're working through our Release 1.1 issues right now. And this is currently the only issue slated for the 1.2 Release which will come directly after that.

Thanks for your patience while we sort through this stuff, lots of tough work to be done!

Thank you for the update!

[AnthonyMDev]

Work is in progress on a complete implementation of this feature. This PR is being closed as it is an insufficient implementation as explained in the above comments. Thanks for your help on getting movement on this issue @tahirmt.