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

Apollo v3.6.8 and above throw Uncaught TypeError: Cannot read properties of undefined (reading 'shift') when running multiple writeFragment calls in a loop #10262

Closed
mogzol opened this issue Nov 4, 2022 · 7 comments · Fixed by #10526

Comments

@mogzol
Copy link

mogzol commented Nov 4, 2022

Intended outcome:

This issue occurs when my app makes multiple calls to client.writeFragment in a loop, like this:

for (const asset of result.model) {
  apolloClient.writeFragment({
    id: asset.id,
    fragment: UPDATE_ASSET_FRAGMENT,
    fragmentName: "UpdateAssetFields",
    data: asset,
  });
}

I would expect this to work fine, and it does work fine on all versions of Apollo prior to v3.6.8

Actual outcome:

On v3.6.8 of Apollo and later, on the second iteration of that loop (so the second writeFragment call), the following error is thrown:

Uncaught TypeError: Cannot read properties of undefined (reading 'shift')
    at Object.complete (Concast.js:42:1)
    at Concast.removeObserver (Concast.js:113:1)
    at ObservableQuery.tearDownQuery (ObservableQuery.js:458:1)
    at ObservableQuery.js:33:1
    at cleanupSubscription (module.js:88:1)
    at Subscription.unsubscribe (module.js:203:1)
    at useQuery.js:115:34
    at HTMLUnknownElement.callCallback (react-dom.development.js:3945:1)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:3994:1)
    at invokeGuardedCallback (react-dom.development.js:4056:1)

How to reproduce the issue:

Unfortunately I'm not entirely sure how to reproduce this (EDIT: see first comment below for reproduction). I tried creating a new basic Apollo project with a single component and query and then running multiple writeFragments, but couldn't get the error to happen. I can tell you that it does happen reliably in my own app, and the change that caused it was this MR: #9791

Specifically the change to ObservableQuery.ts on line 839:

- this.concast.removeObserver(this.observer, true);
+ this.concast.removeObserver(this.observer); 

If I add the second true argument back in, then the error is no longer thrown and everything works properly.

Versions

  System:
    OS: macOS 13.0
  Binaries:
    Node: 16.18.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.19.2 - /usr/local/bin/npm
  Browsers:
    Chrome: 107.0.5304.87
    Firefox: 106.0.2
  npmPackages:
    @apollo/client: 3.6.8 => 3.6.8 
@mogzol
Copy link
Author

mogzol commented Nov 7, 2022

I've been investigating this more and have managed to reproduce the issue, see here:

https://codesandbox.io/s/apollo-typeerror-demo-qygfem?file=/src/App.jsx

The issue seems to occur when there are any observed queries that use the @export directive, which my app relies heavily on. This issue means that this whole use case described in the Apollo documentation does not work on recent Apollo versions.

I've had to downgrade back to Apollo 3.6.7 for the time being, hopefully this issue can be resolved soon so I can upgrade back to the latest version.

It's also worth noting that on React 18 this issue seems to be even more pronounced, it happens immediately, without needing any calls to writeFragment. I'm not sure if it's the exact same issue though, as it also happens on versions older than 3.6.8. Removing the @export directive does still fix it. React 18 demo.

@mogzol mogzol changed the title Apollo v3.6.8 and above throw Uncaught TypeError: Cannot read properties of undefined (reading 'shift') when running multiple writeFragment calls in a loop Apollo v3.6.8 and above throw Uncaught TypeError: Cannot read properties of undefined (reading 'shift')` when running multiple writeFragment calls in a loop Nov 7, 2022
@mogzol mogzol changed the title Apollo v3.6.8 and above throw Uncaught TypeError: Cannot read properties of undefined (reading 'shift')` when running multiple writeFragment calls in a loop Apollo v3.6.8 and above throw Uncaught TypeError: Cannot read properties of undefined (reading 'shift') when running multiple writeFragment calls in a loop Nov 7, 2022
@Camsteack
Copy link

Any update on this ? It's basically not possible to use the @export directive with React 18 ?

@mogzol
Copy link
Author

mogzol commented Jan 30, 2023

I would also appreciate an update on this. This is still preventing me from updating beyond Apollo 3.6.7 or upgrading my app to React 18. I've had a few other issues caused by the @export directive as well, it seems like it isn't a very well tested feature.

@benjamn benjamn self-assigned this Feb 6, 2023
benjamn added a commit that referenced this issue Feb 6, 2023
Fixes issue #10262, since `this.sources.shift()` throws when `complete`
is called called before the `Promise` that should have initialized
`this.sources` resolves, which can happen when the directive combination
`@client @export(as: ...)` is used, because `@export` usage causes a
`Promise<Observable[]>` to be passed to the `Concast` constructor
instead of the `Observable[]` itself.

Although passing a `Promise` to the `Concast` constructor should work,
we only exercise that code path in one place, and the additional wrinkle
of calling `complete` before the `Promise` resolves was never tested:
https://github.com/apollographql/apollo-client/blob/015a1ffff3febe4604956f4bb137a3111f3d8257/src/core/QueryManager.ts#L1226-L1241
benjamn added a commit that referenced this issue Feb 6, 2023
Fixes issue #10262, since `this.sources.shift()` throws when `complete`
is called called before the `Promise` that should have initialized
`this.sources` resolves, which can happen when the directive combination
`@client @export(as: ...)` is used, because `@export` usage causes a
`Promise<Observable[]>` to be passed to the `Concast` constructor
instead of the `Observable[]` itself.

Although passing a `Promise` to the `Concast` constructor should work,
we only exercise that code path in one place, and the additional wrinkle
of calling `complete` before the `Promise` resolves was never tested:
https://github.com/apollographql/apollo-client/blob/015a1ffff3febe4604956f4bb137a3111f3d8257/src/core/QueryManager.ts#L1226-L1241
@benjamn
Copy link
Member

benjamn commented Feb 6, 2023

@mogzol @Camsteack Fix incoming! #10526

@jerelmiller
Copy link
Member

Hey all 👋

It looks like this issue was fixed with 3.7.8 via #10526. I've confirmed this by upgrading the reproduction and verifying the error has gone away. As such, I'm going to go ahead and close this issue. Thanks!

Copy link
Contributor

github-actions bot commented Feb 9, 2024

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants