Skip to content
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

Update to apollo codegen 0.15.0 #566

Closed
wants to merge 2 commits into from

Conversation

BenSchwab
Copy link
Contributor

This is a pre-req for #550

This includes a change apollographql/apollo-tooling@faa5715 which enables:

mergeInFieldsFromFragmentSpreads.

@BenSchwab BenSchwab force-pushed the bschwab--update-apollo-codegen-15 branch from f3296e6 to da5b916 Compare July 12, 2017 21:59
@@ -249,35 +250,63 @@ public void operation_with_fragments() throws Exception {
assertThat(hero.fragments().heroWithFriendsFragment().friends().get(2).fragments().humanWithIdFragment().id()).isEqualTo("1003");
assertThat(hero.fragments().heroWithFriendsFragment().friends().get(2).fragments().humanWithIdFragment().name()).isEqualTo("Leia Organa");

final HumanWithIdFragment supermanAsHumanFragment = new HumanWithIdFragment(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sav007 This is an example of #559

The fact that fragment types are merged in now (and a different type) makes the imperative store api a bit more clumsy and the potential problem -- what happens if a user inconsistently creates the two different fragment objects? Which one will win in the merge process?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I have the same thought while I was implementing write operation. I think it is user responsibility to make sure that fragments with the same id have the same data. If they are different then it depends of field tree traversal

@sav007
Copy link
Contributor

sav007 commented Jul 13, 2017

I see that fields from fragment being merged into query, is it because of logic behind operation id generation? And is there any specs/docs how operation id is generated?

sav007
sav007 previously approved these changes Jul 13, 2017
@@ -14,7 +14,7 @@ fragment starshipFragment on Starship {
pilotConnection {
edges {
node {
...pilotFragment
name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentionally? As this test was used for testing fragment inside another fragment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, was testing something myself and forgot to switch back

@sav007 sav007 dismissed their stale review July 13, 2017 14:42

Have some questions/concerns regarding this

@BenSchwab
Copy link
Contributor Author

@sav007 The field merging is separate from operation id generation. @martijnwalraven May be able to give more insight on it.

I will follow up with another PR to enable the operationId flag, but I wanted to do this PR separately to discuss the field spreading, in case there is any changes we want to do to handle it.

@BenSchwab
Copy link
Contributor Author

apollographql/apollo-tooling#147 This is the PR that enables operationId in Apollo codegen. When we turn the flag on we will get an operationId and source with fragments merged on the IR file (this is different than the fragment spreading we see in this PR) and a build time generated json map file of operationId -> source with fragments

@BenSchwab
Copy link
Contributor Author

In 0.15.2 we can turn off the fragment spread. Looks like there will be no IR file changes until I enable the operationId generation. So going to close this, and proceed work on the operationId change.

@BenSchwab BenSchwab closed this Jul 13, 2017
@BenSchwab BenSchwab deleted the bschwab--update-apollo-codegen-15 branch July 18, 2017 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants