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

Why does GraphQL merge fields across fragment type targets? #399

Closed
mike-marcacci opened this issue Jan 20, 2018 · 4 comments
Closed

Why does GraphQL merge fields across fragment type targets? #399

mike-marcacci opened this issue Jan 20, 2018 · 4 comments

Comments

@mike-marcacci
Copy link
Contributor

mike-marcacci commented Jan 20, 2018

I've recently been fighting with some of the ergonomic downsides to field selection merging – notably field conflict errors and their stifling frequency when dealing with heterogeneous lists and mutations with relay. As I've been thinking through various solutions, I've become curious as to why merging happens at all.

And so that's my question for people with a better knowledge of GraphQL's history: why do we merge fields? I'm sure this was discussed at some point, but I couldn't find it, so I'm going to make a few arguments against merging, with the suspicion that somebody will have a better argument for merging that I missed. :-)

Better Input/Output Symmetry

Input/output symmetry is one of GraphQL's features that helps make it so immediately intuitive. To me, the following query:

query {
	something {
		id

		...on Dog {
			breed
		}

		...on Car {
			make
		}
	}
}

could just as intuitively return:

{
	"something": {
		"id": "9121c30a-941a-4a05-b6ae-c202b6d62522",

		"__on<Dog>": {
			"breed": "Huskey"
		},

		"__on<Car>": {
			"make": "Toyota"
		}
	}
}

as its merged counterpart.

Eliminates Most Field Conflicts

This also greatly reduces the liklihood of field conflicts, meaning queries like the one below can actually be run:

query {
	someGeoQuery {
		id

		...on POI {
			__typename
			coordinates #GeoJSONPointCoordinates
		}

		...on City {
			__typename
			coordinates #GeoJSONPolygonCoordinates
		}
	}
}

Better Interop With Clients & Tools Like Relay

My biggest frustration with the current implementation is not being able to actually reuse fragments when I expect a heterogeneous response. For example, I have an offline-first app where changes are staged in a changeset; this changeset can then be committed when the user is online. Because of precautionary field merge conflicts (not real conflicts, but ones that may exist in the future if I changed a concrete type to be an interface) it's exceedingly difficult to reuse fragments for the mutation response. There are plenty of ways around this, but I'm not sure this is the kind of thing that should need to be worked around.

Simpler Server Implementation

The simplest way to do something is almost always to not do it :-)


Hopefully I've made some interesting points here, and I'm very curious to hear some thoughts in the opposite direction!

@leebyron
Copy link
Collaborator

leebyron commented Mar 6, 2018

Great points - thanks for bringing up the discussion. This is a balancing act where we've tried to find the best combinations of behaviors knowing that there are tradeoffs on either side.

To first answer your core question; why do we merge fields?

We do this so that fragments from different sources may overlap as long as those overlaps agree. Consider this (contrived) example:

fragment A on User {
  friends {
    name
  }
  ...B
}

fragment B on User {
  friends {
    avatar
  }
}

Though the above example is simple, it's a pattern that occurs all the time in complex clients, especially when they co-locate fragments to portions of UI, which is how Relay works. Without field merging this would be an invalid query.

That's also not the most simple example! Here's a much simpler and way more common example of field merging in action:

fragment A on User {
  name
  ...B
}

fragment B on User {
  name
  avatar
}

Without field merging, where would be two fields called name - which one is the correct one? Field merging ensures we result in a single unique mapping of fields requested and the parts of the AST responsible for them.

Just dropping any "duplicate" fields isn't a good enough replacement - both because their subfields may not be equal, and because field arguments might differ. Consider:

fragment A on User {
  avatar(size: SMALL)
  ...B
}

fragment B on User {
  avatar(size: LARGE)
}

What size should avatar be? Field merging that drops repeat field names is not enough to resolve this query. Instead we need to introduce aliases to uniquely identify each field.

To address your counter-proposal of creating nested objects for inline fragments, there are a handful of reasons why we chose not to pursue that direction.

First, support for normalizing clients like Relay and Apollo is a huge reason to keep a 1:1 relationship between JSON object levels in the query result and the object values they represent. For example, seeing the response value in isolation:

{
  "something": {
    "id": "9121c30a-941a-4a05-b6ae-c202b6d62522",
    "__on<Dog>": {
      "breed": "Huskey"
    }
  }
}

Are these two object values with a relationship of some kind? What is the id of the inner value? How should Relay or Apollo normalize and incorporate that data into their store? By creating an intermediate object we've muddied this relationship and made life more challenging for client code. Instead you might image Relay or Apollo needing to insert some additional fields:

{
  something {
    id
    ...on Dog {
      id
      breed
    }
    ...on Car {
      id
      make
    }
  }
}

However now notice that id needed to be repeated at every level - and it will be repeated in the response body as well. While gzip may help clean this up, it will still inflate to a larger memory footprint on the client. This is also a smaller example of an over-fetching problem made worse by the intermediate objects - when fields more complex than id are duplicated fetched, then the response body continues to grow.

Another reason to avoid the intermediate objects is to create the mental model of operating directly on the semantic object values in client code. While clients might be thinking in terms of something.breed they might have to write something['__on<Dog>'].breed because of how they formatted their query. Also the formatting shift from reading ... on Dog in the query but writing __on<Dog> to work with response data would require memorization and would be easy to make a mistake.

Another thing to consider is the semantic relationship between inline fragments and typical fragments. We've framed these two as tightly related such that one can be converted to the other without changing a query's meaning. So the example above should be able to be converted to:

{
  something {
    id
    ...DogFields
    ...CarFields
  }
}

fragment DogFields on Dog {
  breed
}

fragment CarFields on Car {
  make 
}

However either this semantic isomorphism must be broken or to keep the isomorphism, every nested fragment must create an intermediate object. For a highly nested set of fragments like most Relay or Apollo apps, this could quickly make response bodies difficult to navigate.

Also, not every inline fragment needs to condition on a type. Inline fragments can also simply apply directives to a set of selections:

query ($experiment: Boolean) {
  friends { name }
  ... @include(if: $experiment) {
    friends { avatar }
  }
}

If this inline fragment resulted in an intermediate object, then field merging could not merge these two conditions if $experiment is true, and two lists of friends with differing data would be produced, making it harder to understand the result.


There are plenty more patterns that leverage field merging, but hopefully this is a good rounded introduction to why they're necessary and why we keep the 1:1 relationship between object values and objects in the response.

@mike-marcacci
Copy link
Contributor Author

Wow, thanks so much for this fantastic explanation, @leebyron! I hope others find this as helpful as I did.

I think my ideas originated from the way relay presents results to FragmentContainers on the client, essentially simulating my described behavior and bypassing most of the issues of ambiguity you describe here (since each component only gets the fields requested in its fragments). In my attempts to address facebook/relay#2237 I discovered that it can be surprisingly difficult to un-merge fields on the client, and I found myself wishing for more verbosity in the API's response.

However, I definitely agree that field merging makes fragments much more ergonomic for simple clients and non-componentized use cases. Your point about anonymous, non-conditional fragments was also not a case I had considered. And of course, duplicate content adds to the network overhead and memory footprint, as you mentioned.

When I posted this question I figured this ship had sailed as far as the spec is concerned, so I really appreciate you taking the time to lay out these points here.

@mike-marcacci
Copy link
Contributor Author

mike-marcacci commented Mar 6, 2018

Also, as a "food for thought" that I'm not actually planning to pursue, it's occurred to me that merging the fields of inline fragments, but NOT merging named fragments would address most of your points above, while eliminating the possibility of conflict errors in componentized cases like Relay.

It might also be possible to add this kind of behavior with a @scope directive, like:

query {
  something {
    id
    ...DogFields @scope
    ...CarFields @scope
  }
}

or by naming expanded fragments

query {
  something {
    id
    _dogFields: ...DogFields
    _carFields: ...CarFields
  }
}

@jakubriedl
Copy link

Hi @leebyron, thanks for the explanation.
Now I understand why is that happening for interfaces, but is it also necessary for unions?
Because on unions it shouldn't be needed since the objects have nothing in common.

Our use case is that we return an array of components which should be rendered by clients and if they have conflicting prop it is annoying to rename it every time.

demo: https://launchpad.graphql.com/qmr33xm7qp

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

No branches or pull requests

3 participants