-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
🦋 Changeset detectedLatest commit: d6e7b4f 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 |
size-limit report 📦
|
A new release has been made for this PR. You can install it with:
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
/release:pr |
A new release has been made for this PR. You can install it with:
|
|
||
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> |
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 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.
@@ -257,7 +257,7 @@ export abstract class EntityStore implements NormalizedCache { | |||
if (newValue !== fieldValue) { | |||
changedFields[storeFieldName] = newValue; | |||
needToMerge = true; | |||
fieldValue = newValue; | |||
fieldValue = newValue as StoreValue; |
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.
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.
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.
Discussed during our 1:1 - great catch! 🚀
Fixes an issue with the following type:
This PR also makes it possible to return a
DeepPartial
object for fields that are more complex types and don't query for the whole value:This makes it easier to pass a raw server GraphQL parent type to
cache.modify
.