Changed typings of updateQuery's previousQueryResult to be potentially undefined #12276
Conversation
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
|
🦋 Changeset detectedLatest commit: 0c11195 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 |
Cellule
force-pushed
the
update-query-typings
branch
from
3a4c44f to
d9cfe5d
Compare
There was a problem hiding this comment.
I'm generally ok with this change, but would you be willing to add a test that demonstrates the case where previousData is undefined? I'd like to make sure its "documented" in some form through runtime behavior, not just the types. That should help us prevent regressions in the future. That would also help determine whether this is an actual bug, or if this behavior was intentional. I'd like to avoid a band-aid to the types just in case previousData was never intended to be undefined.
Thanks for the contribution!
src/__tests__/subscribeToMore.ts
Outdated
| @@ -241,6 +241,9 @@ describe("subscribeToMore", () => { | |||
| } | |||
| `, | |||
| updateQuery: (prev, { subscriptionData }) => { | |||
| if (!prev) { | |||
There was a problem hiding this comment.
I definitely understand why you did this for this test, but unfortunately this makes it more difficult to know if we introduced regressions by accidentally making prev undefined when it should have a value. The assertion below isn't run in that case, so it would appear that the test would still pass (or at the very least, the source of where the test would fail would be further away from the actual problem).
Instead, I'd recommend updating the prev to prev!, that way this test will crash if this ever switches to undefined.
There was a problem hiding this comment.
Make sense, I wasn't 100% sure how to handle it, but you're right the expectation is that prev should be provided here
There was a problem hiding this comment.
would you be willing to add a test that demonstrates the case where previousData is undefined
If you read the issue, the problem is that we can't figure out the repro steps to have undefined at runtime.
It is likely a race condition that might be hard to reproduce in a testing environment.
I admit I haven't tried recently, this is an issue we've faced years ago and kept the patch.
I can try to get a repro, but I can't make any guarantee
src/__tests__/subscribeToMore.ts
Outdated
| @@ -241,6 +241,9 @@ describe("subscribeToMore", () => { | |||
| } | |||
| `, | |||
| updateQuery: (prev, { subscriptionData }) => { | |||
| if (!prev) { | |||
There was a problem hiding this comment.
Make sense, I wasn't 100% sure how to handle it, but you're right the expectation is that prev should be provided here
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 is
apollo-client/src/core/ObservableQuery.ts Lines 732 to 737 in de8196e That 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 checking |
|
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 In my case I'm using
{
"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
}
} |
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, so I think the right change here might be to wrap As for the other behavior you're seeing, it looks like |
|
There's still something that bugs me with the The scenario seems to be as follows
Real quick it sounds like it would be "safe" to simply not call Another thing to mention, the makeSubscription<
IWorkOrderDetailsQueryData,
IWoMessageUpsertedSubscriptionSubscription,
IWoMessageUpsertedSubscriptionSubscriptionVariables
>("messageUpsertedSubscription", query.subscribeToMore, {
document: messageUpsertedSubscription,
variables: {
parent: { id: workOrderId, type: IUserMessageParentType.WorkOrder },
},
updateQuery(prev, { subscriptionData, variables }) {
const newMessage = subscriptionData?.data?.messageUpserted?.message;
const workOrderUpdates = subscriptionData?.data?.messageUpserted?.parent;
const transcription = subscriptionData.data.messageUpserted?.transcription;
if (
(variables as any as IWorkOrderDetailsQueryVariables).id !==
(workOrderUpdates as any).id
) {
debugger;
return prev;
}
if (!newMessage || workOrderUpdates?.__typename !== "WorkOrder" || !prev?.workOrder) {
return prev!;
} |
|
One issue I see with using 
I wonder if a better alternative is to actually only update the query if it is complete public updateQuery<TVars extends OperationVariables = 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) {
const newResult = mapFn(result, {
variables: (this as any).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 an Looking at the return type of 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? |
Yeah this is exactly why I'm on the fence with the change as well
Humm yeah that could work, but indeed the typings would be the tricky part
Right, I did want to update at least the return type which is more accurate and backward compatible.
Keeping backward compatibility and typescript relation between the 2 variables sounds like a challenge! |
Definitely unfortunate, but I'd wager its more preferable than a crash in production! |
Cellule
force-pushed
the
update-query-typings
branch
from
3188a44 to
179608a
Compare
commit: |
|
Alright here's my attempt This shows my reasoning best I think const updateQuery: Parameters<typeof observable.updateQuery>[0] = jest.fn(
(previousResult) => {
expect(previousResult.complete).toBe(true);
// Type guard
if (!previousResult.complete) {
return undefined;
}
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. I cleaned up a bunch of types that we're not shared where it should Let me know what you think |
Related to #12228
There's an outstanding issue where
previousQueryResultsometimes, flakily, ends upundefinedThis change aims to have the typings reflect the actual runtime behavior allowing devs to write safe code that can handle the missing
previousQueryResultAn alternative could be to simply not call
mapFnifpreviousQueryResultis missing. While neither is ideal nor fixes the underlying issue of "why"previousQueryResultisundefined. At least it would avoid bad crashes.I've been rolling with this patch for years now and I haven't seen any bad side-effects so far. As far as I can tell, the case where
previousQueryResultcan be safely ignored and is likely going to be called again with actual data when it mattersTypings Update for
updateQuery:updateQuery'spreviousQueryResultto be potentially undefined in@apollo/client.updateQuerymethod insrc/core/ObservableQuery.tsto reflect the new typings.UpdateQueryFntype insrc/core/watchQueryOptions.tsto allowpreviousQueryResultto be undefined.ObservableQueryFieldsinterface insrc/react/types/types.tsto reflect the new typings.VSCode Configuration:
editor.codeActionsOnSavesetting to disable organizing imports on save in.vscode/settings.json.