-
Notifications
You must be signed in to change notification settings - Fork 738
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
Add back mergeInFieldsFromFragmentSpreads option #2770
Conversation
@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/ |
👷 Deploy request for apollo-ios-docs pending review.Visit the deploys page to approve it
|
438d478
to
437a6ef
Compare
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. |
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! |
4a117e4
to
d6a1f9d
Compare
@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. |
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! |
Thanks for the update! Happy to help if I can. |
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 SpecificationWe would like to anticipate the addition of other options in the future here, so we want to turn Here is what we have in mind: Add a new property to public let fragmentMergingStrategy: FragmentMergingStrategy The 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 In the future, additional values can be added to the 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 |
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. |
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! |
I changed this PR to be a draft until we're ready for review. Prevents any accidental merging. |
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 |
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! |
d6a1f9d
to
c521307
Compare
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. |
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