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 merge function fails when spreading existing non-normalized data #6245

Closed
3nvi opened this issue May 6, 2020 · 2 comments · Fixed by #6276
Closed

Apollo v3 merge function fails when spreading existing non-normalized data #6245

3nvi opened this issue May 6, 2020 · 2 comments · Fixed by #6276
Assignees
Milestone

Comments

@3nvi
Copy link

3nvi commented May 6, 2020

In Apollo v3 we have the ability to specify a merge function in a particular type field. The merge function works fine when merging types that are normalized in the Cache, but throws an exception if (for whatever reason) you are attempting to spread the existing data.

For example, when defining a merge policy:

 merge: (existing, incoming) => {
    if (!existing) {
        return incoming;
    }

   // dummy one just for showcasing purposes
   return {
     ...existing,
   };
 },

Everything works fine if the type of the existing is something that's normalized in the InMemoryCache. If it's not normalized (by missing an id/ _id or by specifying keyFields: false), then I get the following error:

policies.js:246 Uncaught (in promise) TypeError: Cannot assign to read only property '__typename' of object '[object Object]'
    at policies.js:246
    at Array.forEach (<anonymous>)
    at Policies.push../node_modules/@apollo/client/cache/inmemory/policies.js.Policies.applyMerges (policies.js:245)
    at policies.js:238
    at Array.map (<anonymous>)
    at Policies.push../node_modules/@apollo/client/cache/inmemory/policies.js.Policies.applyMerges (policies.js:238)
    at policies.js:246
    at Array.forEach (<anonymous>)
    at Policies.push../node_modules/@apollo/client/cache/inmemory/policies.js.Policies.applyMerges (policies.js:245)
    at policies.js:246

Funnily enough, if I attempt to spread the incoming instead of the existing, then everything seems fine. I'm including @benjamn since I believe this may relate to his work.

Intended outcome:
Merging of a field should be irrelevant of whether the data is normalized in the cache.

We should be allowed to do:

 merge: (existing, incoming) => {
    if (!existing) {
        return incoming;
    }

   return {
     ...incoming,
     items: [...existing.items, ...incoming.items]
   };
 },

without any issues.

Actual outcome:
If I attempt to write ...existing in the result in the merge function (i.e. ...existing, ...existing.items, etc.) then an error is triggered.

How to reproduce the issue:
It's pretty simple. Define a merge function that returns the example found in the description, for a type that is not normalized in the cache.

Versions

System:
OS: macOS 10.15.2
Binaries:
Node: 10.16.0 - /usr/local/bin/node
Yarn: 1.19.0 - /usr/local/bin/yarn
npm: 6.14.2 - /usr/local/bin/npm
Browsers:
Chrome: 81.0.4044.129
Firefox: 72.0.2
Safari: 13.0.4
npmPackages:
@apollo/client: 3.0.0-beta.41 => 3.0.0-beta.41
apollo-link-error: ^1.1.12 => 1.1.12

@benjamn benjamn self-assigned this May 11, 2020
@benjamn benjamn added this to the Release 3.0 milestone May 11, 2020
benjamn added a commit that referenced this issue May 13, 2020
Almost certainly fixes #6245, though I couldn't find a way to reproduce
the original problem using `...existing` syntax as @3nvi suggested.
@benjamn
Copy link
Member

benjamn commented May 13, 2020

@3nvi Can you give @apollo/[email protected] a try? My fix for this was also in beta.47, but beta.48 is currently the latest version.

@3nvi
Copy link
Author

3nvi commented May 14, 2020

Hey @benjamn the current issue is indeed fixed on @apollo/[email protected], thanks a lot!

Seeing as you worked on data mutations, just wanted to let you know that another issue is still present #6202. I've added a reproduction codesandbox using the latest beta.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants