👷 Deploy request forapollo-client-docs pending review.

Visit the deploys page to approve it

⚠️ Docs preview not attached to branch

The preview was not built because the PR's base branchrelease-3.13 is not in the list of sources.

An Apollo team member can comment one of the following commands to dictate which branch to attach the preview to:

• !docs set-base-branch version-2.6
• !docs set-base-branch main

Build ID: f38bc6ce53417f54a5403a5f

🦋 Changeset detected

Latest commit:b817562

The changes in this PR will be included in the next version bump.

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

If you read the issue, the problem is that we can't figure out the repro steps to have undefined at runtime.

Ah shoot. I read that and it went right over my head. Let me play around with this a bit more and see if I can think of any way this could happen. I'll push a test if I can, or respond back and let you know that I couldn't come up with something.

@Cellule the closest I can get to reproducing this isupdateQuery passing an empty object{} asprevious. Just to double check, the value you're seeing on the crashes isundefined and not access on a nested property whenprevious is{} correct?

updateQuery callscache.diff withreturnPartialData: true:

ThatreturnPartialData: true returns an empty object if there is absolutely no cache data available for the given query. I've never actually seenresult returnundefined and have been unable to reproduce a situation where it returnsundefined instead of{}. I've traced much of the code and am not seeing anything stick out either.

Is there anything else you can tell me about the queries where you see this happen, such as directives used, type policies, etc? Perhaps the combination of one of those things hits this case.

I admit it's been a long time since I've seen the issue because we've mitigated it by always checkingif(!prev) return prev!
I am fairly certainprev === undefined
I'll try to get in a bad state on my side

That would be super helpful if you can. I fear this change is more of a band-aid to the real underlying issue and would love to figure that out if we can. Otherwise this could be a fairly disruptive change to a lot of existing users who could now potentially see a lot of TypeScript errors after upgrading.

Well maybe I was wrong, it seems it might be{} after all (so my patch wouldn't fix anything really)

In my case I'm usingupdateQuery inquery.subscribeToMore
In order to repro I'm doing some operations that will cause the subscription to send more data then navigate to another page "quickly-ish"
The other page is still on the same entity, so it will effectively

• change variables inuseQuery
• ThroughuseEffect it will resubscribe the subscription `
• updateQuery seems to be called with data from the previous variables for a new query with no data (hence empty object)

{"subscriptionData": {"data": {"messageUpserted": {"message": {"id":9627,"content": {"text":"removed the work order from being on hold.","tokens": [                            {"value":"removed the work order from being on hold.","__typename":"TextToken"                            }                        ],"__typename":"TokenizedText"                    },"author": {"id":1338,"img":"https://img.maintainx-dev.com/mx-dev-cac1-uploads/static/user_placeholders/RandomPicture4.png","firstName":"Michael","lastName":"Ferris","displayName":"Michael Ferris","__typename":"User","alternateImg":null,"availabilityStatus":null                    },"createdAt":"2025-01-22T19:50:41.676Z","reactions": [],"__typename":"SystemWorkOrderStatusChangedMessage","extraData": {"oldStatus":"ON_HOLD","newStatus":"OPEN","newStatusVariant":null,"failures":null,"escalatedUsers": [],"escalatedTeams": [],"__typename":"SystemWorkOrderStatusChangedExtraData"                    }                },"parent": {"id":876,// Note the id of the parent that the subscription subscribes to"isUnreadForMe":false,"comments": {"totalCount":0,"unreadCount":0,"__typename":"MessageThread"                    },"__typename":"WorkOrder"                },"transcription":null,"__typename":"MessageUpsertedSubscription"            }        }    },"variables": {"noRedirect":false,"id":4,// Note the id in the variables do not match with the data from the subscription."pagination": {"cursor":"","limit":25        },"shouldFetchAutomationInformation":false    }}

it seems it might be{} after all

Ok great! I was hoping that would be the case and this one actually makes a lot more sense to me. I could reproduce this fairly easily in a test by starting a query, subscribing to the subscription, then having the subscription emit an event before the query finished on the network. In this case, no data had been written to the cache from the query, soprevious would be that empty object.

I think the right change here might be to wrapTData inDeepPartial since its possible that at any given time, you may only get back partial data. It still may ruffle a few feathers, but it would be a more accurate type. Let me converse with@phryneas to make sure we're in alignment, but I'm fairly confident this is the right TypeScript fix. Assuming so, I think it makes most sense to target this change with 3.13 so we can better call it out in the changelog.

As for the other behavior you're seeing, it looks likeupdateQuery is called withthis.variables onObservableQuery, which is just agetter forthis.options.variables. Perhaps this is a race condition between when the options actually change inObservableQuery and when you callsubscribeToMore again? Perhaps you could logobservable.options.variables to see if that value is updated with the new variables.

There's still something that bugs me with thevariables
From what I can test, it's possible the variables passed tosubscribeToMore.updateQuery do not match at all the variables passed to the subscription.
It seems entirely possible for the ObservableQuery to have receive new variables, but the subscription is still on the old variables

The scenario seems to be as follows

• Variables are changed (navigation or whatever)
• useQuery receives new variables and updates the underlying ObservableQuery
• useEffect(() => query.subscribeToMore(...), [query.variables]) causes the subscription to be slated to be disconnected
• Data arrives in the subscription, theupdateQuery is called for the previous variables, but the cache looks for query data using the new variables (leading to{})
• useEffect unsubscribes the subscription then resubscribes correctly
  It looks like a race condition, but so far it hasn't been hard to reproduce on my side

Real quick it sounds like it would be "safe" to simply not callmapFn inupdateQuery ifresults === {} || results === undefined.
So far I haven't been able to reproduce the variables mismatch when there's a cache hit, but it sounds possible in theory

Another thing to mention, theSubscribeToMoreOptions.updateQuery implies the variables will have typeTSubscriptionVariables but in reality, the variables returned areTVariables (not currently passed in the generics).
This type is simply wrong, while it's unlikely to be used, if the variables of the query and the variable of the subscription differ enough, it can cause bugs. I had to cast it just to confirm the bad state

makeSubscription<IWorkOrderDetailsQueryData,IWoMessageUpsertedSubscriptionSubscription,IWoMessageUpsertedSubscriptionSubscriptionVariables>("messageUpsertedSubscription",query.subscribeToMore,{document:messageUpsertedSubscription,variables:{parent:{id:workOrderId,type:IUserMessageParentType.WorkOrder},},updateQuery(prev,{ subscriptionData, variables}){constnewMessage=subscriptionData?.data?.messageUpserted?.message;constworkOrderUpdates=subscriptionData?.data?.messageUpserted?.parent;consttranscription=subscriptionData.data.messageUpserted?.transcription;if((variablesasanyasIWorkOrderDetailsQueryVariables).id!==(workOrderUpdatesasany).id){debugger;returnprev;}if(!newMessage||workOrderUpdates?.__typename!=="WorkOrder"||!prev?.workOrder){returnprev!;}

One issue I see with usingDeepPartial is that it will then become a typings nightmare to properly return the correct object

I wonder if a better alternative is to actually only update the query if it is complete
I had that idea in the beginning because in my opinion, if for whatever reason the previous data is missing or incomplete, I will want to no-op the update.
It does make a difference in the runtime and could potentially affect some legit use case (although I can't think of one) I feel it might be the better/safer approach.
Likely, if you have a good reason to want to write in the cache of an incomplete query, you're more likely to callcache.writeQuery manually than rely onupdateQuery

publicupdateQuery<TVarsextendsOperationVariables=TVariables>(mapFn:(previousQueryResult:Unmasked<TData>,options:Pick<WatchQueryOptions<TVars,TData>,"variables">)=>Unmasked<TData>|undefined):void{const{queryManager}= this;const{result,complete}=queryManager.cache.diff<Unmasked<TData>>({query:this.options.query,variables:this.variables,returnPartialData:true,optimistic:false,});if(complete&&result){constnewResult=mapFn(result,{variables:(thisasany).variables,});if(newResult){queryManager.cache.writeQuery({query:this.options.query,data:newResult,variables:this.variables,});queryManager.broadcastQueries();}}}

I go back and forth on this. This sounds like a reasonable change, but I fear its too much of a breaking change for a patch/minor release. Since we don't provide any additional information in the callback (i.e. whether the previous result is complete), there could be cases where suddenly your callback is no longer called and its difficult to tell why.

That said, the callback does take anoptions argument as the 2nd argument. Perhaps we can provide thecomplete flag here and let the user decide whether to use the partial data to return a full result or to ignore the update (i.e. returnundefined). At least this gives the best of both worlds and puts it into the user's hands on how to handle it.

Looking at the return type ofupdateQuery, currently its set toUnmasked<TData>, but it does appear the implementation isok returning a falsey value and will do nothing if thats the case. We should probably reflect this in the types as well by updating the return type toUnmasked<TData> | undefined so that TypeScript allows this.

The tricky part would be updating the TypeScript type here. It would be ideal if it were something like this:

updateQuery(previous,{ complete}){if(complete){previous;// ^? TData}previous;// ^? DeepPartial<TData>}

Thoughts on this?

I go back and forth on this. This sounds like a reasonable change, but I fear its too much of a breaking change for a patch/minor release. Since we don't provide any additional information in the callback (i.e. whether the previous result is complete), there could be cases where suddenly your callback is no longer called and its difficult to tell why.

Yeah this is exactly why I'm on the fence with the change as well

That said, the callback does take anoptions argument as the 2nd argument. Perhaps we can provide thecomplete flag here and let the user decide whether to use the partial data to return a full result or to ignore the update (i.e. returnundefined). At least this gives the best of both worlds and puts it into the user's hands on how to handle it.

Humm yeah that could work, but indeed the typings would be the tricky part

Looking at the return type ofupdateQuery, currently its set toUnmasked<TData>, but it does appear the implementation isok returning a falsey value and will do nothing if thats the case. We should probably reflect this in the types as well by updating the return type toUnmasked<TData> | undefined so that TypeScript allows this.

Right, I did want to update at least the return type which is more accurate and backward compatible.
This is why I have to do this in my code currentlyupdateQuery(prev) { if (!prev) return prev!; ... }

The tricky part would be updating the TypeScript type here. It would be ideal if it were something like this:

updateQuery(previous,{ complete}){if(complete){previous;// ^? TData}previous;// ^? DeepPartial<TData>}

Thoughts on this?

Keeping backward compatibility and typescript relation between the 2 variables sounds like a challenge!
I'll try to give it some thoughts, but whatever solution it is likely to cause friction after the update, which I think is unavoidable

whatever solution it is likely to cause friction after the update, which I think is unavoidable

Definitely unfortunate, but I'd wager its more preferable than a crash in production!

npm i https://pkg.pr.new/@apollo/client@12276

commit:b817562

Alright here's my attempt

This shows my reasoning best I think

constupdateQuery:Parameters<typeofobservable.updateQuery>[0]=jest.fn((previousResult)=>{expect(previousResult.complete).toBe(true);// Type guardif(!previousResult.complete){returnundefined;}return{user:{ ...previousResult.user,name:"User (updated)"}};});

There's no way I can think of to bind the type of a parameter with a type guard on another parameter.
The details can be tweaked, but my idea is to attach acomplete property straight on the result. Obviously this doesn't work as-is since the result could have acomplete property.
In the callback, if you don't check forprev.complete the type will bePartial<Unmasked<TData>> (I gave up onDeepPartial because it was giving me typescript errors due to type being too complex)

I cleaned up a bunch of types that we're not shared where it should
I fixed the typings forvariables in the options to beTVariables and added a newsubscriptionVariables: TSubscriptionVariables in the subscribeToMore case

Let me know what you think

This is turning out to be a much more difficult change than expected 🙃.

Unfortunately adding acomplete field to the result itself is a no-go. We want to avoid doing anything withdata since it could conflict with the fields returned from the GraphQL result. We do our best to avoid that at all costs.

Unfortunately changing that first argument in any capacity is a breaking change. That said, how urgent is this change for you? We have started working on v4 and we could certainly think about moving this change there. I'd be open to redesigning theupdateQuery API to be more type-safe and v4 would give us a chance to make any necessary breaking changes to facilitate this.

Thoughts?

Alright I think it mostly looks good.
If you think there are other things that needs to be fixed I won't mind if you decide to take care of it yourself. I will likely not have more time before next week as I'm working on a presentation for Fragment Masking to my company this Friday :)

Sounds great! Really appreciate the back and forth. I'll make sure this change makes it into the 3.13 beta 🙂.

Hope the presentation goes well! Feel free to ping me on Discord (@jerelmiller) if you've got questions about data masking 🙂. I'd love to answer them!