Assume addTypename:true, but hide implicit __typename result fields. #5154
Conversation
As @jbaxleyiii pointed out in this comment, the root query and mutation types do not necessarily have to be called "Query" or "Mutation", and the only way to find their real names is to ask for the __typename property: #5146 (comment)
Following up on some intentions I recently expressed on Twitter: https://twitter.com/benjamn/status/1157422802359730176 This implementation is a bit more nuanced than simply forcing the addTypename option to be true, since that approach yields a lot of unwanted __typename fields in result trees. Instead, if we exclude those automatically-injected __typename fields from query results, application code will receive only the __typename information that it asked for explicitly, and the cache will always receive the __typename information that it needs.
benjamn
changed the title
There was a problem hiding this comment.
Great stuff @benjamn - looks good! 👍 from me after test fixes of course, but those are probably just mismatches between the mocked responses having or not having __typename, and not lining up with the expected results - should be quick to fix.
|
Those test failures run a bit deeper than I hoped. Specifically, a surprising number of tests rely on Somewhat fewer tests (but still more than I expected) are written in a way that expects I'm not entirely sure how representative our test suite is of actual application code, but I'm afraid the strategy in this PR needs some more thought. |
|
@benjamn I'm quite familiar with those tests and I feel quite confident that 1) they are not indicative of application code patterns and 2) usage of |
|
Furthermore, with our thinking around a different parsing strategy all together, allowing runtime setting of query manipulation is not in our best interest and I think should be removed as this PR is doing. |
|
@jbaxleyiii Thanks for the confirmation that people don't really want/expect @hwillson For the purposes of restructuring the repo, don't worry about merging this PR before you get started. I plan on revising this approach substantially when I get back from my vacation, so merge conflicts are not a concern. |
|
With regards to adding the |
This change was adapted from #5154, per @jbaxleyiii's approval: #5154 (comment) If you read from the cache using a query that does not have explicit __typename fields, the cache should not return data containing the __typename field. Unfortunately, we have a bunch of tests that currently enforce the accidental presence of __typename, so this relatively simple change required updating a lot of test code.
|
Closing in favor of #5320… and also because I don't want to deal with those merge conflicts. |
As @jbaxleyiii pointed out in this comment, the root query and mutation types do not necessarily have to be called "Query" or "Mutation", and the only way to find their real names is to ask for their
__typenamefields. In other words, when adding__typenamefields to a query, we should make sure the root operation has a__typenamefield, just like any other selection set. Hence the first commit in this PR: 4377e02.Until now, the
InMemoryCachehas allowed application developers to decide if the cache should add__typenameto the queries it processes. While this might seem like a reasonable configuration option, the truth is that the cache always needs__typenameinformation, and the only legitimate reason to passaddTypename: falseis when you (the application developer) don't want to receive__typenamefields in your query results when you didn't ask for them.It turns out we can satisfy the latter goal by simply excluding any implicitly-injected
__typenamefields from cache results. With that objection out of the way, it becomes much less disruptive to forceaddTypename: trueas the default behavior, which allows the cache to rely on the presence of type information without visibly polluting cache results returned to the application.