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

Component not re-render in apollo-link-state after upgrading apollo-cache-inmemory to 1.3.* #3992

Closed
laukaichung opened this issue Oct 11, 2018 · 33 comments
Assignees

Comments

@laukaichung
Copy link

laukaichung commented Oct 11, 2018

Intended outcome:

I use apollo link state to save an array of objects to the local client. In apollo-cache-inmemory 1.2.10, the component is able to re-render on pushing new items to the array or deleting one item from the array. After upgrading to 1.3.5, I expected everything would stay the same.

Actual outcome:

However the component fails to re-render at all with 1.3.*.

How to reproduce the issue:

apollo-cache-inmemory 1.2.10:

https://codesandbox.io/s/l99l9r1ml9 (There's an issue with codesandbox. I have to double click the buttons to trigger the re-render, but on my local, there's no such issue.)

apollo-cache-inmemory 1.3.5

https://codesandbox.io/s/8xqn9mz0l (It doesn't work at all, even if I double click either buttons)

Versions

  "dependencies": {
    "graphql": "0.13.2",
    "react": "16.5.0",
    "apollo-cache-inmemory": "1.3.5",
    "apollo-link-state": "^0.4.2",
    "apollo-link-http": "1.5.5",  
    "react-apollo": "2.2.4",
    "react-dom": "16.2.0",
    "apollo-client": "^2.4.2",
    "graphql-tag": "^2.10.0"        
  },
@JoviDeCroock
Copy link
Contributor

Your first and second link have the same behavior for me.

I'm using the latest chrome version, i don't know if this is relevant but just throwing it out there.

@laukaichung
Copy link
Author

@JoviDeCroock
Have you tried double clicking on the "Create" button at https://codesandbox.io/s/l99l9r1ml9
I don't know why but codesandbox doesn't re-render on first click. It works fine on the localhost.

@JoviDeCroock
Copy link
Contributor

Yes, maybe i should mention this the same behavior is that they both are working.

@laukaichung
Copy link
Author

laukaichung commented Oct 11, 2018

@JoviDeCroock
Just an update.
Looks like 1.3.5 doesn't work on Chromium (Arch Linux) with this example: https://codesandbox.io/s/8xqn9mz0l
It works on mobile Chrome.

However,
I've also tried the 1.3.5 version in react native but it also doesn't work. I see that the array has been updated in the memory, but the component just doesn't automatically re-render. No errors shown in the console.

@JoviDeCroock
Copy link
Contributor

That's a pretty odd issue, is there some way to reproduce it consistently for others aswell?

@laukaichung
Copy link
Author

laukaichung commented Oct 11, 2018

One more update:
The problem only occurs when it is an array. If the data is a string or integer, the re-rendering works as usual.
Example: https://codesandbox.io/s/71zm7m35o6

@carllippert
Copy link

There is some info about this and how state changes are broadcasted in the local-state slack channel provided by @benjamn

@carllippert
Copy link

Like to slack comment about a possible work around. https://apollographql.slack.com/archives/C87A16E8Y/p1539111592000100

@laukaichung
Copy link
Author

laukaichung commented Oct 12, 2018

@carllippert
Would you please post the workaround here? I cannot create an account to read. I do not have an @meteor.com email.

@carllippert
Copy link

image

@benjamn
Copy link
Member

benjamn commented Oct 16, 2018

As a general note, you shouldn't be mutating the results returned from the cache, unless you are careful to re-write them immediately, since those objects may be returned for future reads. Make copies of the data you read from the cache, if you plan on changing the contents of the objects.

If I'm diagnosing this problem correctly, perhaps we should be Object.freeze-ing cache results?

@OurMajesty
Copy link

Having the same issue.
1.2.10 - no problem, 1.3.5 did not rerender.
Instructions on screenshot didn't help.

@OurMajesty
Copy link

OurMajesty commented Oct 16, 2018

1.3.0-beta.15 did work, BTW
sorry, mistaken. beta doesn't work either

@benjamn
Copy link
Member

benjamn commented Oct 16, 2018

@OurMajesty Can you put together a small reproduction?

@OurMajesty
Copy link

OurMajesty commented Oct 16, 2018

Thank you for response, I'll try.

As a general note, you shouldn't be mutating the results returned from the cache

Tried to change push to concat and assignment to object spread, but no luck.

@OurMajesty
Copy link

OurMajesty commented Oct 16, 2018

https://codesandbox.io/s/q7rkm6y1ow
something like this, but in real app I add new item to cache inside update callback of my backend mutation
1.2.10: items are added without a problem
1.3.5: items aren't added

@laukaichung
Copy link
Author

laukaichung commented Oct 17, 2018

@benjamn @OurMajesty
I have tried concat to clone the array in the 1.35 example, but still the component doesn't update.

The only way it can be worked, is to clear the cached data, then save the new data in React Native and Chromium. Like this (Example: https://codesandbox.io/s/wnxmzmm7o7):

        cache.writeData({
          data: {
            filterList: []
          }
        });
        cache.writeData({
          data: {
            filterList: [...filterList]
          }
        });

@joshjg
Copy link
Contributor

joshjg commented Oct 18, 2018

@benjamn If that's the case (we shouldn't be mutating the cache results) then we need to update the docs, which are mutating the cache results.
https://www.apollographql.com/docs/react/features/optimistic-ui.html#optimistic-advanced

So if this is expected behavior, then it is surely a breaking change, right?

@dmarkow
Copy link
Contributor

dmarkow commented Oct 18, 2018

Similar issue here going from 1.2.10 to 1.3.5, though I'm using readQuery/writeQuery in the update callback of a mutation. I was using .push to add something to a nested array and writing the data back, but nothing re-rendered. I now have to provide a new top level object to get it to re-render (which I can do with an immutability library, but will require updating hundreds of callbacks in our app).

I also did this based on the documentation examples mutating the cache results:

update: (proxy, { data: { createTodo } }) => {
      const data = proxy.readQuery({ query });
      data.todos.push(createTodo);
      proxy.writeQuery({ query, data });
    }

@ctretyak
Copy link

If I do writeQuery in update method of mutation It doesn't call rerender of my component with query like in writeQuery

benjamn added a commit that referenced this issue Oct 20, 2018
Should help fix #3992.

As #3992 (and specifically the reproduction that @OurMajesty provided at
https://codesandbox.io/s/q7rkm6y1ow) demonstrates, since the InMemoryCache
may return the same object for multiple reads (when the data have not
changed), the previousResult is often exactly (===) the same object as
newData.result, which was causing the code removed by this commit to
return early instead of broadcasting the watch.

This early return was unsafe, because the contents of the object may have
changed since they were previously broadcast, so we actually do need to
report those modifications, even though the object reference is the same.

In other words, `previousResult === newData.result` does not imply
"nothing has changed," as I mistakenly assumed in this discussion:
#3394 (comment)

In the longer term, I would like to eliminate the previousResult logic
entirely, since it seems redundant now that we can precisely track changes
to the store, but for now it's important for backwards compatibility.
Specifically, previousResult still provides a way to prefer the previous
object if it is structurally identical to the new object, though that
preference is moot when `previousResult === newData.result`.

The previousResult object should never be used as a basis for skipping
broadcasts. Only the application developer can decide whether === object
identity means it's safe to skip rendering, likely in the context of a
larger design which treats cache results as immutable data. The job of the
InMemoryCache is to make no unsound assumptions, and broadcast results
whenever they may have changed.
benjamn added a commit that referenced this issue Oct 20, 2018
Should help with #3992.

Though deep cloning and isEqual testing are obviously more expensive than
relying on === equality, the reality is that lastResult may have been
modified since it was previously recorded, so === equality is useless for
determining isDifferentResult. A defensive copy is the only way to make
sure we get this right.
@benjamn
Copy link
Member

benjamn commented Oct 20, 2018

Fix incoming with #4032.

@benjamn
Copy link
Member

benjamn commented Oct 22, 2018

I believe this has been fixed with apollo-client@2.4.3 and apollo-cache-inmemory@1.3.6. To confirm, I updated @OurMajesty's reproduction by changing just those versions, and everything seems to work now.

Please try updating those packages, and comment here with your findings (good or bad)!

@OurMajesty
Copy link

OurMajesty commented Oct 23, 2018

UPD: That was my problem, ensure you update both apollo-client and apollo-cache-inmemory

So, it worked, thanks! But i had to change some code, to ensure I don't mutate anything like I did before.

@OurMajesty
Copy link

nevermind, the problem was because I didn't update apollo-client to 2.4.3, just apollo-cache-inmemory
thanks again

@laukaichung
Copy link
Author

laukaichung commented Oct 23, 2018

I have an issue with updating an array of object in cache in 1.3.6. The component will only re-render when the logs are cloned before unshifting a new item to the array.

Working example:

        <Mutation
            update={(cache, {data: {createLog}}) => {
                let {logs} = cache.readQuery( {query,variables});
                logs = [...logs];
                if (logs) {
                    logs.unshift(createLog);
                    cache.writeQuery({
                        query,
                        variables,
                        data: {logs}
                    });
                }
            }}
            mutation={someMutation}/>

The component will not re-render when the logs are cloned after unshifting. How could this happen? Same things happen on Chromium and Mobile Chrome.

        <Mutation
            update={(cache, {data: {createLog}}) => {
                let {logs} = cache.readQuery( {query,variables});
                if (logs) {
                    logs.unshift(createLog);
                    cache.writeQuery({
                        query,
                        variables,
                        data: {logs:[...logs]}
                    });
                }
            }}
            mutation={someMutation}/>

@benjamn
Copy link
Member

benjamn commented Oct 23, 2018

@stonecold123 Can you update your CodeSandbox reproduction above to demonstrate the issue?

@benjamn benjamn self-assigned this Oct 23, 2018
@cagdastulek
Copy link

@benjamn 1.3.6 did not fix a similar issue we had after upgrading to 1.3.5. We don't have the issue with 1.2.10. We observed that changing direct fields of an object works just fine but any field with nested fields (in our case with two level nesting) causes issues. The data we get from query was not changed.
Example:

data.foo = {
  x: 5,
  y: "bar",
  z: {
    a: "5"
    b: {
       field1: "value1"
       field2: "value2"
    }
  }
}

Changes to x, y are gine, but changes to z.b.field1 are not reflected back. I can provide a test case if you like.

@benjamn
Copy link
Member

benjamn commented Oct 23, 2018

@cagdastulek Please do throw together a CodeSandbox reproduction.

@bogdansoare
Copy link

having the same issue

@dmarkow
Copy link
Contributor

dmarkow commented Oct 26, 2018

https://codesandbox.io/s/k027kz5243

I'm doing something very similar to the data.todos.push example in the docs.

The sandbox example is using apollo-link-state instead of an actual API endpoint. If I update the data in the link state's resolver for the mutation using cache.writeQuery, the component re-renders (see the commented out code in index.js).

However, if I comment that code out and just return the newly added item (similar to what the frontend would see as a response from our API) and instead use writeQuery in the update callback in the mutation (see List.js), the component doesn't re-render. If you change apollo-cache-inmemory to 1.2.10, it works fine, but it fails on 1.3.6.

I also threw a logging line in the update callback to check the data. My contacts list is getting updated (each time I click the button, the count goes up by one if you check the console), it's just not re-rendering.

benjamn added a commit that referenced this issue Oct 26, 2018
This commit is a more conservative version of
e66027c

We still need to make a deep clone of observableQuery.lastResult in order
to determine if future results are different (see #3992), but we can
restrict the use of that snapshot to a single method, rather than
replacing observableQuery.lastResult with the snapshot.

Should help with #4054.
benjamn added a commit that referenced this issue Oct 30, 2018
This commit is a more conservative version of
e66027c

We still need to make a deep clone of observableQuery.lastResult in order
to determine if future results are different (see #3992), but we can
restrict the use of that snapshot to a single method, rather than
replacing observableQuery.lastResult with the snapshot.

Should help with #4054.
benjamn added a commit that referenced this issue Oct 30, 2018
This commit is a more conservative version of
e66027c

We still need to make a deep clone of observableQuery.lastResult in order
to determine if future results are different (see #3992), but we can
restrict the use of that snapshot to a single method, rather than
replacing observableQuery.lastResult with the snapshot.

Should help with #4054 and #4031.
@chentsulin
Copy link
Contributor

chentsulin commented Oct 31, 2018

I also ran into the similar issue when reading nested fields from the store. Everything in the store is correct after calling a mutation, but still getting the old data in the Query component.

After diving into node_modules/apollo-cache-inmemory, I find out that it gets correct new data from the store when I removed the following lines:

this.executeStoreQuery = wrap((options: ExecStoreQueryOptions) => {
return executeStoreQuery.call(this, options);
}, {
makeCacheKey({
query,
rootValue,
contextValue,
variableValues,
fragmentMatcher,
}: ExecStoreQueryOptions) {
// The result of executeStoreQuery can be safely cached only if the
// underlying store is capable of tracking dependencies and invalidating
// the cache when relevant data have changed.
if (contextValue.store instanceof DepTrackingCache) {
return reader.cacheKeyRoot.lookup(
reader.keyMaker.forQuery(query).lookupQuery(query),
contextValue.store,
fragmentMatcher,
JSON.stringify(variableValues),
rootValue.id,
);
}
}
});
this.executeSelectionSet = wrap((options: ExecSelectionSetOptions) => {
return executeSelectionSet.call(this, options);
}, {
makeCacheKey({
selectionSet,
rootValue,
execContext,
}: ExecSelectionSetOptions) {
if (execContext.contextValue.store instanceof DepTrackingCache) {
return reader.cacheKeyRoot.lookup(
reader.keyMaker.forQuery(execContext.query).lookupSelectionSet(selectionSet),
execContext.contextValue.store,
execContext.fragmentMatcher,
JSON.stringify(execContext.variableValues),
rootValue.id,
);
}
}
});
}

@benjamn
Copy link
Member

benjamn commented Oct 31, 2018

@dmarkow I believe your problem is fixed in apollo-client@2.4.5, thanks presumably to #4069.

@chentsulin Not sure I can help you without a reproduction. Please make sure you're using the latest versions of apollo-client, apollo-cache-inmemory, apollo-utilities, react-apollo, etc.

@dmarkow
Copy link
Contributor

dmarkow commented Oct 31, 2018

@benjamn It sure does. Thank you!

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

No branches or pull requests