[codegen] import directive #236
Conversation
👷 Deploy request for eclectic-pie-88a2ba pending review.Visit the deploys page to approve it
|
…renced Fragment includes an import directive
| self.referencedFragments = referencedFragments | ||
| self.source = source | ||
| self.filePath = filePath | ||
| self.isLocalCacheMutation = directives? | ||
| .contains { $0.name == Constants.DirectiveNames.LocalCacheMutation } ?? false | ||
|
|
||
| let referencedImports: [Directive] = self.referencedFragments |
There was a problem hiding this comment.
I see what you're doing here, but I don't think we should be adding the referenced fragment's @import directives to the parent's directives. We should add them to the moduleImports but not the actual directives property.
There was a problem hiding this comment.
This means moduleImports probably just becomes a stored property instead of being computed.
| .flatMap { $0 } | ||
| self.directives = Array(Set(directives ?? [] + referencedImports)) | ||
|
|
||
| self.moduleImports = self.directives? |
There was a problem hiding this comment.
I'm also wondering if we should alphabetize these so that changing the order of Fragment Spreads doesn't change the order of import statements. That could be confusing when you change something in one Fragment definition and then a bunch of operations have their import statements change.
I see the use of Set when setting up the directives here. But if we are going to not add the referenced fragment imports to the directives, we should probably just make moduleImports an OrderedSet to ensure we don't get duplicate import statements.
| self.referencedFragments = .fromJSValue(jsValue["referencedFragments"], bridge: bridge) | ||
| self.source = jsValue["source"] | ||
| self.filePath = jsValue["filePath"] | ||
|
|
||
| let directives: [Directive]? = .fromJSValue(jsValue["directives"], bridge: bridge) |
There was a problem hiding this comment.
Same as above. We should also try to break this into a static function so that we can re-use the logic instead of repeating it in both places.
| } | ||
|
|
||
| var debugDescription: String { | ||
| return "Import \"\(moduleName)\"" |
There was a problem hiding this comment.
| return "Import \"\(moduleName)\"" | |
| return "@import(module: \"\(moduleName)\")" |
| @@ -7,7 +7,7 @@ enum TemplateTarget: Equatable { | |||
| /// Used in schema types files; enum, input object, union, etc. | |||
| case schemaFile(type: SchemaFileType) | |||
| /// Used in operation files; query, mutation, fragment, etc. | |||
| case operationFile | |||
| case operationFile(moduleImports: [String]?) | |||
There was a problem hiding this comment.
Can we make this have a default value of nil so we don't need to provide moduleImports: nil in all of the unit tests?
I can't remember if you can do default values on enum case associated values? 🤔
…les in definitions + do not assing to the definitions directives anymore
…les in definitions + do not assing to the definitions directives anymore
Hello @AnthonyMDev thank you for reviewing the PR. I applied the changes based on your review. In terms of the tests i was not sure where to add them unfortunately. I would greatly appreciate it if you can add some. Thank you in advance. |
…les in definitions + do not assing to the definitions directives anymore
| "revision" : "937e904258d22af6e447a0b72c0bc67583ef64a2", | ||
| "version" : "1.0.4" | ||
| "revision" : "d029d9d39c87bed85b1c50adee7c41795261a192", | ||
| "version" : "1.0.6" |
There was a problem hiding this comment.
OrderedSet was not Sendable (resulting in a warning) in version 1.0.4 so i updated collections to 1.0.6 as it was fixed there.
There was a problem hiding this comment.
That's great. If we need that patch version in order for this to compile correctly, then we should probably make it the minimum version number in the Package.swift file too.
| @@ -255,6 +279,18 @@ public final class CompilationResult: JavaScriptObjectDecodable { | |||
| public static func ==(lhs: FragmentDefinition, rhs: FragmentDefinition) -> Bool { | |||
| return lhs.name == rhs.name | |||
| } | |||
|
|
|||
| private static func getImportModuleNames(directives: [Directive]?, | |||
There was a problem hiding this comment.
I can add this as a global function in order to avoid any code duplication. For now i left this helper inside the corresponding class.
…les in definitions + do not assing to the definitions directives anymore
…les in definitions + do not assing to the definitions directives anymore
…les in definitions + do not assing to the definitions directives anymore
There was a problem hiding this comment.
This all looks great! I'm going to leave this PR pending until I can take some time to write the unit tests for it. If you get the opportunity to use this branch with your project, it would be great to get some real-world testing too.
I'll get the tests done and have this shipped for the next release we make. And I'll be sure that's before the end of the month for you!
| "revision" : "937e904258d22af6e447a0b72c0bc67583ef64a2", | ||
| "version" : "1.0.4" | ||
| "revision" : "d029d9d39c87bed85b1c50adee7c41795261a192", | ||
| "version" : "1.0.6" |
There was a problem hiding this comment.
That's great. If we need that patch version in order for this to compile correctly, then we should probably make it the minimum version number in the Package.swift file too.
|
Ill double check to see if we can use the fork on our side @AnthonyMDev. Thank you again! Edit: Codegen on our side works as expected in my branch :). The imports are added to all files generated, related to the operations. |
This PR handles the new import directive.
If a graphql operation adds this directive, an import statement to the corresponding generated source file and the source of its referenced fragments will be added.