Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/pink-shoes-cheer.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Fix a bug where GraphQL variable default values were not applied during cache reads when variables with defaults were explicitly set to `undefined`. This caused `@include`/`@skip` directives to throw "Invalid variable referenced" errors when the variable was passed as `undefined` instead of being omitted entirely.
55 changes: 55 additions & 0 deletions src/cache/inmemory/__tests__/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,61 @@ describe("reading from the store", () => {
});
});

it("applies defaults when variable is explicitly undefined", () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great test! Can we also verify it correctly uses defaults if show is omitted altogether as well? I don't think we have any other test in this suite that verifies that. That just makes sure we don't add any regressions there as well 🙂

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think the test doesn't work at all since the field has no value in the store. So even if show: true was used (or defaulted), the query would return only { id: "abcd" } as the result?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ab-pm the readQueryFromStore helper that this test uses sets returnPartialData: false by default so if @include really weren't evaluated, the result would be null instead of { id: "abcd" } due to the result being a partial result. I think its ok 🙂

const query = gql`
query someQuery($show: Boolean = false) {
id
field @include(if: $show)
}
`;

const variables = {
show: undefined,
};

const store = defaultNormalizedCacheFactory({
ROOT_QUERY: {
__typename: "Query",
id: "abcd",
} as StoreObject,
});

const result = readQueryFromStore(reader, {
store,
query,
variables,
});

expect(result).toEqual({
id: "abcd",
});
});

it("applies defaults when variables are omitted entirely", () => {
const query = gql`
query someQuery($show: Boolean = false) {
id
field @include(if: $show)
}
`;

const store = defaultNormalizedCacheFactory({
ROOT_QUERY: {
__typename: "Query",
id: "abcd",
} as StoreObject,
});

const result = readQueryFromStore(reader, {
store,
query,
});

expect(result).toEqual({
id: "abcd",
});
});

it("runs a nested query", () => {
const result: any = {
id: "abcd",
Expand Down
6 changes: 2 additions & 4 deletions src/cache/inmemory/readFromStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {
FragmentMapFunction,
} from "@apollo/client/utilities/internal";
import {
compact,
DeepMerger,
getDefaultValues,
getFragmentFromSelection,
Expand Down Expand Up @@ -204,10 +205,7 @@ export class StoreReader {
}: DiffQueryAgainstStoreOptions): Cache.DiffResult<T> {
const policies = this.config.cache.policies;

variables = {
...getDefaultValues(getQueryDefinition(query)),
...variables!,
};
variables = compact(getDefaultValues(getQueryDefinition(query)), variables);

const rootRef = makeReference(rootId);
const execResult = this.executeSelectionSet({
Expand Down
Loading