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

TypePolicy merge function called before cache deep merge #9503

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gastonmorixe
Copy link

@gastonmorixe gastonmorixe commented Mar 8, 2022

Issue #9502 based on v3.5.10

@gastonmorixe gastonmorixe force-pushed the gm/fix/merge-function-called-before-deepmerge branch from 6644576 to 045a9d5 Compare March 8, 2022 19:11
const merge = typePolicy.merge;
if (merge) {
mergeTree.info = {
field: undefined as any,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what field should be set here, I don't think we need a field in this case.

@@ -660,6 +675,9 @@ export class StoreWriter {
}

if (mergeTree.info) {
if(isReference(existing) && isReference(incoming)){
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be we should check if they are the same ref too?

@benjamn benjamn self-requested a review March 9, 2022 00:48
@benjamn benjamn added this to the Release 3.6 milestone Mar 9, 2022
@benjamn benjamn added 💸 caching 🔍 investigate Investigate further labels Mar 9, 2022
@benjamn
Copy link
Member

benjamn commented Mar 9, 2022

@gastonmorixe Thanks for diving into this thorny area of the code, and for all the time you must have spent debugging. I owe you a more thorough review, but I appreciate the clarity of your proposed solution. We will get this sorted out before v3.6 is released (in mid-March).

@gastonmorixe
Copy link
Author

Thank you @benjamn

@Raphael-Schulz
Copy link

@benjamn @gastonmorixe Has this issue been solved in 3.6.0?
We are also experiencing the same problems described here #9502

@Raphael-Schulz
Copy link

Or is there maybe a workaround that we can use until then? 😀

@gastonmorixe
Copy link
Author

@benjamn update on this? It's working for us quite well in production

@prsauer
Copy link

prsauer commented Oct 5, 2022

@benjamn @gastonmorixe is this going anywhere? merge isn't useable for quite a few cases until this is resolved...

@gastonmorixe
Copy link
Author

gastonmorixe commented Oct 11, 2022

@benjamn @gastonmorixe is this going anywhere? merge isn't useable for quite a few cases until this is resolved...

Well, in the meantime you can use my PR/branch. Google how to use patch-package or check the releases in our fork. We update them every few months to keep in sync.

Yes, I agree without this fixed Apollo Client would be useless for us. That's why we took like a week to find a fix, we couldn't afford not having this solved after all our app is based on this framework. Which mostly we love.

@benjamn updates? Thank you!

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

Successfully merging this pull request may close these issues.

5 participants