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
|
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 126f5a5c81a60f85bbd31129 |
🦋 Changeset detectedLatest commit: 6af8e64 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!
| @@ -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.
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.