-
Notifications
You must be signed in to change notification settings - Fork 2.9k
More robust handling of local state default value when read function is defined #12934
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
Changes from all commits
5c2627a
c9974f7
8e11de8
281d673
f418f0c
19d993e
6081aff
a2dace2
4a9f40a
9ec3941
2530b8b
826efef
6557f43
896478b
fa5145a
dc29907
7b56986
1950e6f
28f138d
3fbf754
4065958
a976196
738659b
0888622
846783f
4bceff8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@apollo/client": minor | ||
| --- | ||
|
|
||
| Don't set the fallback value of a `@client` field to `null` when a `read` function is defined. Instead the `read` function will be called with an `existing` value of `undefined` to allow default arguments to be used to set the returned value. | ||
|
|
||
| When a `read` function is not defined nor is there a defined resolver for the field, warn and set the value to `null` only in that instance. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@apollo/client": patch | ||
| --- | ||
|
|
||
| Ensure `LocalState` doesn't try to read from the cache when using a `no-cache` fetch policy. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@apollo/client": patch | ||
| --- | ||
|
|
||
| Warn when using a `no-cache` fetch policy without a local resolver defined. `no-cache` queries do not read or write to the cache which meant `no-cache` queries are silently incomplete when the `@client` field value was handled by a cache `read` function. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@apollo/client": minor | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. Would like opinions on whether this should be viewed as a minor or patch. |
||
| --- | ||
|
|
||
| Add an abstract `resolvesClientField` function to `ApolloCache` that can be used by caches to tell `LocalState` if it can resolve a `@client` field when a local resolver is not defined. | ||
|
|
||
| `LocalState` will emit a warning and set a fallback value of `null` when no local resolver is defined and `resolvesClientField` returns `false`, or isn't defined. Returning `true` from `resolvesClientField` signals that a mechanism in the cache will set the field value. In this case, `LocalState` won't set the field value. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (CJS)": 44542, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production) (CJS)": 39461, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\"": 33696, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 27707 | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (CJS)": 44753, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production) (CJS)": 39420, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\"": 33901, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 27727 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,13 @@ import { | |
| } from "@apollo/client/testing/internal"; | ||
| import { InvariantError } from "@apollo/client/utilities/invariant"; | ||
|
|
||
| const WARNINGS = { | ||
| MISSING_RESOLVER: | ||
| "Could not find a resolver for the '%s' field nor does the cache resolve the field. The field value has been set to `null`. Either define a resolver for the field or ensure the cache can resolve the value, for example, by adding a 'read' function to a field policy in 'InMemoryCache'.", | ||
| NO_CACHE: | ||
| "The '%s' field resolves the value from the cache, for example from a 'read' function, but a 'no-cache' fetch policy was used. The field value has been set to `null`. Either define a local resolver or use a fetch policy that uses the cache to ensure the field is resolved correctly.", | ||
| }; | ||
|
|
||
| describe("General functionality", () => { | ||
| test("should not impact normal non-@client use", async () => { | ||
| const query = gql` | ||
|
|
@@ -632,7 +639,7 @@ describe("Cache manipulation", () => { | |
| }); | ||
|
|
||
| expect(read).toHaveBeenCalledTimes(1); | ||
| expect(read).toHaveBeenCalledWith(null, expect.anything()); | ||
| expect(read).toHaveBeenCalledWith(undefined, expect.anything()); | ||
| expect(console.warn).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
@@ -1510,3 +1517,149 @@ test("throws when executing subscriptions with client fields when local state is | |
| ) | ||
| ); | ||
| }); | ||
|
|
||
| test.each(["cache-first", "network-only"] as const)( | ||
| "sets existing value of `@client` field to undefined when read function is present", | ||
| async (fetchPolicy) => { | ||
| const query = gql` | ||
| query GetUser { | ||
| user { | ||
| firstName @client | ||
| lastName | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const read = jest.fn((value = "Fallback") => value); | ||
| const client = new ApolloClient({ | ||
| cache: new InMemoryCache({ | ||
| typePolicies: { | ||
| User: { | ||
| fields: { | ||
| firstName: { | ||
| read, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| link: new ApolloLink(() => { | ||
| return of({ | ||
| data: { user: { __typename: "User", lastName: "Smith" } }, | ||
| }).pipe(delay(10)); | ||
| }), | ||
| localState: new LocalState(), | ||
| }); | ||
|
|
||
| await expect( | ||
| client.query({ query, fetchPolicy }) | ||
| ).resolves.toStrictEqualTyped({ | ||
| data: { | ||
| user: { __typename: "User", firstName: "Fallback", lastName: "Smith" }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(read).toHaveBeenCalledTimes(1); | ||
| expect(read).toHaveBeenCalledWith(undefined, expect.anything()); | ||
| } | ||
| ); | ||
|
|
||
| test("sets existing value of `@client` field to null and warns when using no-cache with read function", async () => { | ||
| using _ = spyOnConsole("warn"); | ||
| const query = gql` | ||
| query GetUser { | ||
| user { | ||
| firstName @client | ||
| lastName | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const read = jest.fn((value) => value ?? "Fallback"); | ||
| const client = new ApolloClient({ | ||
| cache: new InMemoryCache({ | ||
| typePolicies: { | ||
| User: { | ||
| fields: { | ||
| firstName: { | ||
| read, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| link: new ApolloLink(() => { | ||
| return of({ | ||
| data: { user: { __typename: "User", lastName: "Smith" } }, | ||
| }).pipe(delay(10)); | ||
| }), | ||
| localState: new LocalState(), | ||
| }); | ||
|
|
||
| await expect( | ||
| client.query({ query, fetchPolicy: "no-cache" }) | ||
| ).resolves.toStrictEqualTyped({ | ||
| data: { | ||
| user: { __typename: "User", firstName: null, lastName: "Smith" }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(read).not.toHaveBeenCalled(); | ||
| expect(console.warn).toHaveBeenCalledTimes(1); | ||
| expect(console.warn).toHaveBeenCalledWith( | ||
| WARNINGS.NO_CACHE, | ||
| "User.firstName" | ||
| ); | ||
| }); | ||
|
|
||
| test("sets existing value of `@client` field to null and warns when merge function but not read function is present", async () => { | ||
| using _ = spyOnConsole("warn"); | ||
| const query = gql` | ||
| query GetUser { | ||
| user { | ||
| firstName @client | ||
| lastName | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| const merge = jest.fn(() => "Fallback"); | ||
| const client = new ApolloClient({ | ||
| cache: new InMemoryCache({ | ||
| typePolicies: { | ||
| User: { | ||
| fields: { | ||
| firstName: { | ||
| merge, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }), | ||
| link: new ApolloLink(() => { | ||
| return of({ | ||
| data: { user: { __typename: "User", lastName: "Smith" } }, | ||
| }).pipe(delay(10)); | ||
| }), | ||
| localState: new LocalState(), | ||
| }); | ||
|
|
||
| await expect(client.query({ query })).resolves.toStrictEqualTyped({ | ||
| data: { | ||
| user: { | ||
| __typename: "User", | ||
| firstName: "Fallback", | ||
| lastName: "Smith", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(merge).toHaveBeenCalledTimes(1); | ||
| expect(merge).toHaveBeenCalledWith(undefined, null, expect.anything()); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a |
||
|
|
||
| expect(console.warn).toHaveBeenCalledTimes(1); | ||
| expect(console.warn).toHaveBeenCalledWith( | ||
|
jerelmiller marked this conversation as resolved.
|
||
| WARNINGS.MISSING_RESOLVER, | ||
| "User.firstName" | ||
| ); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I marked this as
minor(in which case we'll want to repoint the branch) since it feels like enough of a change to justify a minor. Happy to update topatchthough if it feels more like a bug fix. Opinions welcome.