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

Fix issue returning arrays from cache.modify modifier functions when the array contains a union type #11994

Merged
merged 6 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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/grumpy-pandas-pump.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Allow `cache.modify` to return deeply partial data.
5 changes: 5 additions & 0 deletions .changeset/shaggy-lions-brake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

Prevent accidental distribution on `cache.modify` field modifiers when a field is a union type array.
54 changes: 54 additions & 0 deletions src/cache/core/__tests__/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,5 +517,59 @@ describe.skip("Cache type tests", () => {
},
});
});

test("Allow for mixed arrays on union fields", () => {
const cache = new TestCache();
cache.modify<{
union: Array<
| { __typename: "Type1"; a: string }
| { __typename: "Type2"; b: string }
>;
}>({
fields: {
union(field) {
expectTypeOf(field).toEqualTypeOf<
ReadonlyArray<
| Reference
| { __typename: "Type1"; a: string }
| { __typename: "Type2"; b: string }
>
>();
return field;
},
},
});
});

test("Allows partial return data", () => {
const cache = new TestCache();
cache.modify<{
union: Array<
| { __typename: "Type1"; a: string; c: { foo: string } }
| { __typename: "Type2"; b: string; d: { bar: number } }
>;
}>({
fields: {
union(field) {
expectTypeOf(field).toEqualTypeOf<
ReadonlyArray<
| Reference
| {
__typename: "Type1";
a: string;
c: { foo: string };
}
| {
__typename: "Type2";
b: string;
d: { bar: number };
}
>
>();
return [{ __typename: "Type1", a: "foo" }];
},
},
});
});
});
});
5 changes: 3 additions & 2 deletions src/cache/core/types/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type {
StoreValue,
isReference,
AsStoreObject,
DeepPartial,
} from "../../../utilities/index.js";

import type { StorageType } from "../../inmemory/policies.js";
Expand Down Expand Up @@ -107,12 +108,12 @@ export type ModifierDetails = {
export type Modifier<T> = (
value: T,
details: ModifierDetails
) => T | DeleteModifier | InvalidateModifier | undefined;
) => DeepPartial<T> | DeleteModifier | InvalidateModifier | undefined;

type StoreObjectValueMaybeReference<StoreVal> =
StoreVal extends Array<Record<string, any>> ?
StoreVal extends Array<infer Item> ?
Item extends Record<string, any> ?
[Item] extends [Record<string, any>] ?
ReadonlyArray<AsStoreObject<Item> | Reference>
Copy link
Member Author

@jerelmiller jerelmiller Aug 7, 2024

Choose a reason for hiding this comment

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

I think it eventually makes sense to also pass a DeepPartial as the value to the Modifier function since we can never guarantee you have data for the full type at any given time, only what has been written.

Doing this now might create a bit of churn though, so we might want to wait for a minor/major to make this change as fields that do property access with dot-notation might start complaining once we introduce this. I don't know what kind of impact that will have on our users, but might be good to avoid some anger for now.

: never
: never
Expand Down
2 changes: 1 addition & 1 deletion src/cache/inmemory/entityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ export abstract class EntityStore implements NormalizedCache {
if (newValue !== fieldValue) {
changedFields[storeFieldName] = newValue;
needToMerge = true;
fieldValue = newValue;
fieldValue = newValue as StoreValue;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed since the DeepPartial returns unknown on types that it doesn't know what to do with. StoreValue is a union of primitives and Reference so its unable to determine how to apply it.


if (__DEV__) {
const checkReference = (ref: Reference) => {
Expand Down
Loading