-
Notifications
You must be signed in to change notification settings - Fork 61
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
Fixing fragment definition generation when not generating operation definitions #218
Conversation
Only generate definitions in fragment files if the 'operationDocumentFormat' config contains the 'definition' value.
✅ Deploy Preview for eclectic-pie-88a2ba ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
I'm confused how the tests are passing here. fragmentDefinition
is a requirement of the Fragment
protocol, so generating a fragment without this property should make it not conform to the Fragment
protocol.
@AnthonyMDev Going to assume it has to do with it just doing string comparison of what gets generated, more tests would still need to be added to cover actual compilation before this patch would be ready to go. Mainly got this up so we could have a discussion about how/if we wanted to handle this |
Ah I see. Yes, I do think this is the right approach. We don't need to separate configuration option for disabling fragment definitions, it should use the We just need to figure out the right way to make this compile. We could make the The Codegen Test Configurations CI job actually generates the code, compiles a project using it and runs some tests. So maybe we need to add a configuration there that has this option turned off. |
Yea I think a default implementation for |
-Adding default fragment protocol implementation -Adding codegen configuration test to validate compilation
@@ -13,7 +13,8 @@ | |||
}, | |||
"pruneGeneratedFiles" : true, | |||
"queryStringLiteralFormat" : "multiline", | |||
"warningsOnDeprecatedUsage" : "include" | |||
"warningsOnDeprecatedUsage" : "include", | |||
"operationDocumentFormat" : ["operationId"] |
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.
We don't need to address this in this current PR. But for integration testing individual configuration options, maybe we could build something better here.
We need to have these multiple different project setups in TestCodeGenConfigurations
for testing the actual project configurations (Cocoapods, SPM, Xcode) and codegen methods (Relative vs absolute, schema module embedded, etc).
These are things that affect how/where the files are generated and if they include the correct namespacing and import statements to link and compile properly.
But for testing other individual configuration properties which would be the same regardless of these configurations, we should probably have a single project that we re-generate with different configurations so that we can isolate and test just the configuration options we want to test. These could be done using the Swift Scripting codegen to make it easier to run multiple of them rapidly.
This will take some time to set up properly, so it's not something I think we need to do right now. But this isn't the first time I've noticed that we are kind of overloading these integration tests, so it might be worth thinking about.
/// Extension providing default implementation for the ``Fragment`` protocol. | ||
extension Fragment { | ||
// Default implementation for the `fragmentDefinition` variable | ||
static var fragmentDefinition: StaticString { |
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 need to be public
?
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.
I think the failing CI test is because of this actually.
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.
It does indeed, forgot to mark it public
45b05486 Fixing fragment definition generation when not generating operation definitions (#218) git-subtree-dir: apollo-ios-codegen git-subtree-split: 45b054866bb3307b3b941dcf62dff144a706599c
…ration when not generating operation definitions git-subtree-dir: apollo-ios-codegen git-subtree-mainline: ed0f372 git-subtree-split: 45b054866bb3307b3b941dcf62dff144a706599c
Only generate definitions in fragment files if the 'operationDocumentFormat' config contains the 'definition' value.
Closes apollographql/apollo-ios#3282