-
Notifications
You must be signed in to change notification settings - Fork 60
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
Field Merging[1/x] Configuration Options #306
Conversation
✅ Deploy Preview for eclectic-pie-88a2ba canceled.
|
✅ Deploy Preview for apollo-ios-docc canceled.
|
1353191
to
eec8762
Compare
eec8762
to
f9cdce4
Compare
f9cdce4
to
88084be
Compare
88084be
to
5748c50
Compare
5748c50
to
86fec35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, had a couple questions.
@@ -1351,6 +1483,7 @@ extension ApolloCodegenConfiguration.OutputOptions { | |||
deprecatedEnumCases: ApolloCodegenConfiguration.Composition = Default.deprecatedEnumCases, | |||
schemaDocumentation: ApolloCodegenConfiguration.Composition = Default.schemaDocumentation, | |||
selectionSetInitializers: ApolloCodegenConfiguration.SelectionSetInitializers = Default.selectionSetInitializers, | |||
fieldMerging: ApolloCodegenConfiguration.FieldMerging = Default.fieldMerging, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for adding this parameter to the initializer parameters? Couldn't we just be using Default.fieldMerging
down in the body of the initializer? That would maintain the signature and functionality both as before and if a user wants the new functionality then they must use the new initializer?
@@ -169,6 +169,7 @@ extension ComputedSelectionSet { | |||
fileprivate func finalize() -> ComputedSelectionSet { | |||
let merged = MergedSelections( | |||
mergedSources: mergedSources, | |||
mergingStrategy: .all, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change in a future PR to use the codegen configuration value?
86fec35
to
3cbbd5e
Compare
…tringly typed Codable usage for JSON encoding
3cbbd5e
to
41f58c4
Compare
Closed in favor of #431 |
TL;DR
Update the configuration in ApolloCodegen to allow for selecting specific field merging strategies.
What changed?
Added the ability to select specific field merging strategies such as ancestors, siblings, and named fragments in the ApolloCodegen configuration.
How to test?
Unit tests have been added to ensure encoding and decoding of field merging configurations work correctly.
Why make this change?
This change provides more flexibility in handling merged fields and named fragment accessors in the generated selection set models.