-
Notifications
You must be signed in to change notification settings - Fork 39
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 referencedFragments to CompilationResult #39
Add referencedFragments to CompilationResult #39
Conversation
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.
expect(fragment?.referencedFragments).to(equal(expected)) | ||
} | ||
|
||
func test__referencedFragments__PetDetails_isCorrect() throws { |
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.
func test__referencedFragments__PetDetails_isCorrect() throws { | |
func test__referencedFragments__PetDetails_shouldNotHaveReferencedFragments() throws { |
Is something like that a clearer name for the test? isCorrect
can mean many things.
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.
These are kind of odd tests because they test the example animal kingdom API. When that API, these tests need to be changed to accurately represent the correct information there. So changing the name also just doesn't add value. The only reason "shouldNotHaveReferencedFragments" is true is because the Pet Details fragment doesn't have any right now. It's not truly a test that "a fragment with no referenced fragments should have an empty referenced fragments array." The unit tests for this PR are the tests in the JS code. These are just some added integration tests.
expect(operation?.referencedFragments).to(equal(expected)) | ||
} | ||
|
||
func test__referencedFragments__HeightInMeters_isCorrect() throws { |
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.
Is this test meant to be different from the test__referencedFragments__PetDetails_isCorrect
test below?
"WarmBloodedDetails", | ||
"PetDetails" | ||
].map { expectedName in | ||
compilationResult.fragments.first(where:{ $0.name == expectedName })! |
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.
This check looks like it's enforcing that the expected fragment name must be found but should the test also be validating that there are only the three referenced fragments?
Allow us to track the referenced fragments from the js front end. This means we have access to this information prior to running the
IRBuilder.build
functions.We can now generate operation identifiers and manifests with this information without having to build the entire IR first.
This could also be used in the future to track dependencies between fragments to optimize the order of building (and releasing from memory) the fragments in the codegen.