Prevent incorrect result merging when aliasing __typename or id.
#10789
Conversation
🦋 Changeset detectedLatest commit: f27932d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
phryneas
changed the title
__typename or id.
src/utilities/graphql/storeUtils.ts
Outdated
| for (const selection of selectionSet.selections) { | ||
| if (!isField(selection)) { |
There was a problem hiding this comment.
I have a feeling getTypenameFromResult is (or could be) hot code, since we need to know the __typename for every result object, and each call might involve recursive fragment traversal.
Instead of iterating over the selections again here, here's one way to avoid the whole second loop when there are no fragments in a given selection set:
diff --git a/src/utilities/graphql/storeUtils.ts b/src/utilities/graphql/storeUtils.ts
index f995b41d9..f2ddec155 100644
--- a/src/utilities/graphql/storeUtils.ts
+++ b/src/utilities/graphql/storeUtils.ts
@@ -18,6 +18,7 @@ import {
NameNode,
SelectionSetNode,
DocumentNode,
+ FragmentSpreadNode,
} from 'graphql';
import { isNonNullObject } from '../common/objects';
@@ -289,18 +290,26 @@ export function getTypenameFromResult(
selectionSet: SelectionSetNode,
fragmentMap?: FragmentMap,
): string | undefined {
+ let fragments: undefined | Array<InlineFragmentNode | FragmentSpreadNode>;
+
for (const selection of selectionSet.selections) {
if (isField(selection)) {
if (selection.name.value === '__typename') {
return result[resultKeyNameFromField(selection)];
}
+ } else if (fragments) {
+ fragments.push(selection);
+ } else {
+ fragments = [selection];
}
}
+
if (typeof result.__typename === 'string') {
return result.__typename;
}
- for (const selection of selectionSet.selections) {
- if (!isField(selection)) {
+
+ if (fragments) {
+ for (const selection of fragments) {
const typename = getTypenameFromResult(
result,
getFragmentFromSelection(selection, fragmentMap)!.selectionSet,| const specifierOrId = keyFn(object, context); | ||
| const specifierOrId = keyFn({...object, ...storeObject}, context); |
There was a problem hiding this comment.
Great catch @phryneas!
For future reference, I think this should have been keyFn(storeObject, context) from the beginning, but our test suite has a few test cases where properties of a result object were important (like id) even if not mentioned as fields in the query document, so keyFn({...object, ...storeObject}, context) ensures backwards compatibility for that edge case, while always preferring storeObject properties over object properties.
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This fixes the first of two bugs pointed out in #10784.
This is just a draft PR, not the finished result, and tons of tests fill fail right now. I got the correct behavior in for this case, but broke a ton of other stuff in the meantime and will need to work on that.
Just opening the PR for transparency here :)
Checklist: