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

Regression in 3.9.5 with cache updates and error responses #11759

Closed
jerelmiller opened this issue Apr 5, 2024 · 10 comments · Fixed by #11839
Closed

Regression in 3.9.5 with cache updates and error responses #11759

jerelmiller opened this issue Apr 5, 2024 · 10 comments · Fixed by #11839
Assignees
Labels

Comments

@jerelmiller
Copy link
Member

From #11579 (comment)

Unfortunately, I was unable to achieve an error regarding the missing field using the codesandbox example, so I reproduced a simpler case with a field merge error in "typePolicies" (index.js:22).

I'm attaching two identical codesandboxes, the only difference being the versions of @apollo/client.

@apollo/client v.3.9.5
https://codesandbox.io/p/sandbox/issue-apollo-client-v-3-9-5-notes-app-vk8jlf

@apollo/client v.3.9.4
https://codesandbox.io/p/sandbox/issue-apollo-client-v-3-9-5-notes-app-forked-d3923z

Steps to reproduce:

  1. optional, restart the backend codesandbox if it is needed
  2. Visit the sandbox.
  3. Add a Todo.
  4. Update the added Todo.

Actual behavior (in version 3.9.5 - broken):
After updating the Todo, the label of the Todo is NOT updated.

Expected behavior (in version 3.9.4 - works):
After updating the Todo, the label of the Todo is updated.

P.S. At first glance, it may seem strange why I presented a code example where I intentionally broke cache management functionality. There are two main concerns on my part:

  1. Applications running smoothly but likely having inaccuracies in cache configuration (typePolicies) up to version 3.9.4 inclusively worked seamlessly.
  2. Given that there was an error in the configuration of field merge settings in "typePolicies", it remains impossible to easily detect the error. Neither the developer console nor the application provides any information about the problem whatsoever.
@iamdylancurran
Copy link

Just want to follow up on this, from 3.9.4 to 3.9.5+ we are experiencing updates to the cache failing to apply and it's breaking many of our unit tests.

This only happens if we upgrade from 3.9.4 forward, and the issues seems to match the example you gave in the initial comment. There's no error or explanation given so we have no idea what we're actually meant to fix here, and we can't upgrade as it stands.

@jerelmiller
Copy link
Member Author

Thanks for letting us know @iamdylancurran! I can almost certainly guarantee its related to #11579. This fixed a bug where a partial cache update could be reported to a query that didn't accept partial updates, but seems it may have caused other side effects. No guarantees on a timeline, but we'll see if we can look at this soon.

@jonnyleeharris
Copy link

jonnyleeharris commented Apr 25, 2024

Just want to follow up on this, from 3.9.4 to 3.9.5+ we are experiencing updates to the cache failing to apply and it's breaking many of our unit tests.

This only happens if we upgrade from 3.9.4 forward, and the issues seems to match the example you gave in the initial comment. There's no error or explanation given so we have no idea what we're actually meant to fix here, and we can't upgrade as it stands.

I am having the exact same issue. Some e2e tests started failing - pinned it down to a cache update not occurring.

We do not use returnPartialData or notifyOnNetworkStatus.

We do make heavy use of fragments though, and are using @graphql-codegen to generate our type policies/fragment matchers.

I will report back if i find anything useful :)

@jerelmiller
Copy link
Member Author

Hey @av-k 👋

I've been digging into this a bit more to understand the change that happened. From what I'm seeing, it looks like 3.9.4 works because the partial cache write from one query/mutation causes a network request for the other query which will then fulfill the cache and re-render with the full data set. The change I introduced in 3.9.5 skips this step, so it explains why this broke for you. My learning here is that some partial cache writes are ok even if the query doesn't tolerate it since it will perform a network request to fulfill the whole result in that situation.

I think the proper thing for us to do instead would be to only avoid the partial cache broadcast on queries that had previously errored, but allow it on queries without errors to ensure that network request happens.

That said, would you be willing to provide me with a bit more "real world" reproduction of the bug? While the example you gave me properly demonstrates the issue, I'd argue that the reproduction is a misuse of the merge function since its returning a different data type than the schema allows. I know this was likely only for reproduction purposes, but I'd like to write a unit test for this in our codebase that better represents the actual scenerio that is causing the breakage.


@iamdylancurran and @jonnyleeharris does the above sound like the situation you're seeing as well? If either of you have a scenerio that I can use as a test case, that would also be awesome.

@jerelmiller
Copy link
Member Author

Scratch that, I think I was able to get a believable situation for my tests. I've opened #11839 which I believe addresses this issue.

@av-k @iamdylancurran @jonnyleeharris would you be able to try the following snapshot release to see if it addresses your issue?

npm i @apollo/client@0.0.0-pr-11839-20240510211204

@av-k I've tried that snapshot release on your reproduction and it seems to fix the issue: https://codesandbox.io/p/sandbox/issue-apollo-client-v-3-9-5-notes-app-forked-qcn3ct?file=%2Fpackage.json%3A10%2C25

@av-k
Copy link

av-k commented May 13, 2024

Hi @jerelmiller, thank you for the update!

Firstly, I reverted to my source code at the point when I initially encountered the incorrect behavior with @apollo/client versions 3.9.5 and 3.10.3. I then took several steps within my app to ensure that the issue was reproducible with the current versions.

Next, I installed npm i @apollo/client@0.0.0-pr-11839-20240510211204 and repeated the steps within my app to confirm whether the issue no more persisted with this specific version.

Following these steps, I can confirm that the issue has been resolved.

Is there anything else I can assist you with, @jerelmiller?
Once again, thank you for the fix!

@jerelmiller
Copy link
Member Author

Awesome to hear. Thanks @av-k! At this point I should just need a code review from the team to get it in. I'll try and get this in the next patch version. Thanks for confirming!

@jerelmiller jerelmiller self-assigned this May 13, 2024
Copy link
Contributor

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.

@iamdylancurran
Copy link

@jerelmiller Can confirm your fix solved our issue and we've updated to the latest release now, thank you 🙏

@jerelmiller
Copy link
Member Author

Glad to hear it! Thanks for confirming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants