Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

When variables change, data is the previous result in Query Component #2202

Closed
1 of 5 tasks
asafigan opened this issue Jul 21, 2018 · 71 comments · May be fixed by #2889
Closed
1 of 5 tasks

When variables change, data is the previous result in Query Component #2202

asafigan opened this issue Jul 21, 2018 · 71 comments · May be fixed by #2889
Labels
has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository

Comments

@asafigan
Copy link

I am using the "cache-and-network" policy.

Intended outcome:
Rerender Query with different variables. Data is {} until new request is resolved from cache or network.

Actual outcome:
Rerender Query with different variables. Returns previous data until next query is resolved without error.

How to reproduce the issue:
https://stackblitz.com/edit/react-snlwzf
change the input and hit submit.

Version

  • apollo-client@
  • react-apollo@
  • has-reproduction
  • feature
  • docs
  • blocking
  • good first issue

Backgroud
I only want to display a loading state if I don't have a response from the cache or network, but I do want to show loading state when variables change and I don't have a response from cache. Right now it is impossible to tell if data is the response from cache or if it is the previous result. Loading property is no help because it is allows true until the network request is resolved.

@ghost ghost added the has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository label Jul 21, 2018
@jcready
Copy link

jcready commented Jul 25, 2018

Yeah, you basically have to use something like this to prevent rendering data from a query with different variables:

if (data.reddit && data.reddit.subreddit.name !== this.state.search) {
  data = {};
}

And that only works if the data being returned also includes the variable you passed in. Definitely seems like a bug.

@sbrichardson
Copy link

sbrichardson commented Jul 25, 2018 via email

@jcready
Copy link

jcready commented Jul 26, 2018

@sbrichardson the problem is that OP wants to see cached data for query when the variable is the same, but the problem is that Apollo is displaying the cached results for wrong query if the cached results for the current query do not already exist in the cache.

The expected behavior is for Apollo to provide an empty data object when the results for the current query do not exist in the cache.

The actual behavior is that Apollo is providing the previous data object when the results for the current query do not exist in the cache.

Please see a more detailed example here.

@asafigan
Copy link
Author

I do think that it can be valuable to show the previous result while the new one is loading. I have a search auto complete feature that does this. But I also need this case. I can't check for equality of a name or id because the query is a search query. The variables that are passed into the function child are the same as the current input so I can't check equality using that.

@mike-marcacci
Copy link
Contributor

@jcready - thanks for the clear description. I suppose that both behaviors could be desirable in different situations, and it’s a matter of which is default.

With the suggested strategy it would be pretty pretty trivial to restore the current behavior by creating a component stores the Query’s variables/results in state and only updates them once the variables have changed and data is present.

Changing the existing behavior in userland is a bit trickier, given that we don’t know which variables the current data is from.

I agree that this is a bug.

@jcready
Copy link

jcready commented Jul 26, 2018

This issue also looks to be duplicated in #1342 and #1963

I believe the source of this issue is around this bit of logic in the <Query/> component:

react-apollo/src/Query.tsx

Lines 391 to 400 in 2788981

if (loading) {
Object.assign(data.data, this.previousData, currentResult.data);
} else if (error) {
Object.assign(data, {
data: (this.queryObservable!.getLastResult() || {}).data,
});
} else {
Object.assign(data.data, currentResult.data);
this.previousData = currentResult.data;
}

Specifically line 392. Since a query with different variables will not have cached data, but will have previous data from the previous query with different variables, the data will end up being just the previous data. I would argue that solution is to reset this.previousData in the componentWillReceiveProps() method when shallowEqual(nextProps.variables, this.props.variables) is not true.

To support pagination perhaps it is necessary to have the component accept some new prop which would either accept an array of variable keys which would invalidate the previousData if changed or perhaps just accept a function which would act similar to a react component's shouldComponentUpdate(). I imagine something like:

<Query
  query={query}
  variables={variables}
  fetchPolicy="cache-and-network"
  shouldInvalidatePreviousData={(nextVariables, previousVariables) =>
    nextVariables.subreddit !== previousVariables.subreddit
  }/>

@asafigan
Copy link
Author

I agree that shouldInvalidatePreviousData should be a function that defaults to shallow equal. I implemented a simple replacement of the Query component that used that strategy and it worked well.

@marano
Copy link

marano commented Oct 13, 2018

I've just stumbled across this bug. No news if this is going to be fixed?

@brunoreis
Copy link

brunoreis commented Nov 20, 2018

This is very weird. I'm almost sure I've seen this working well before in another app, but I'm facing this now too. It is most likely a matter of how the query is being cached. The cache's PK needs to be the query plus variables.

@brunoreis
Copy link

brunoreis commented Nov 20, 2018

Looking at the cache I do see different entries for different variable sets:

screen shot 2018-11-20 at 17 03 11

@brunoreis
Copy link

first discovery here: if you use refetch instead of changing the props from the parent, the networkStatus (https://www.apollographql.com/docs/react/api/react-apollo.html#graphql-query-data-networkStatus) will be 2 that's more appropriate than 1. If you pass from the parent it will be one.

@brunoreis
Copy link

I agree with @jcready. That's probably the error due to the behavior I've observed here.

I spent a lot of hours yesterday trying to find a workaround and this is the best I could come up with. I prefer using the graphql hocs approach together with recompose functions. Please adjust the solution to your scenario.

The best way I could find to show correct data was to check if the query really has a cache and then to infer that the data is from the cache instead of from the previous variable set.

I created this method to check for the cache:

const queryCacheExists = (client, query, variables) => {
  try {
    client.readQuery({
      query,
      variables,
    });
    return true;
  } catch (e) {
    return false;
  }
};

And then this logic:

const hasItems = props.queryData.queryField ? props.queryData.queryField.edges.length > 0 : false;
      const queryLoading = props.queryData.loading;
      const showingCache = (
        hasItems
        && queryLoading
        && queryCacheExists(props.client, MY_QUERY, props.queryData.variables)
      );

So, after that, showingCache will indicate if the query is showing cached items. And then I can use a custom loading logic like:

loading: queryLoading && !showingCache,

If someone has any simpler solution, please let us know here on this issue.

@enriquemorenotent
Copy link

I suspect I have the same problem, but with Mutation. I posted it here.

@maximeg
Copy link

maximeg commented Dec 15, 2018

Encountered the same issue.

Solution:

  • the proposed shouldInvalidatePreviousData can be a generic solution
  • we could also simply not use previousData in the loading phase when fetchPolicy contains use of cache, because that's a double useless caching layer, with side effect as we can see here.

Relevent code:

react-apollo/src/Query.tsx

Lines 396 to 402 in 386b6d7

if (loading) {
Object.assign(data.data, this.previousData, currentResult.data);
} else if (error) {
Object.assign(data, {
data: (this.queryObservable!.getLastResult() || {}).data,
});
} else {

@oeed
Copy link

oeed commented Dec 24, 2018

Is there any work around for this or plans to fix it?

@fdhani
Copy link

fdhani commented Feb 24, 2019

any update on this issue, this is a pretty big problem imo

@adrianos10
Copy link

adrianos10 commented Feb 27, 2019

I also encountered this problem using HOCs. It only starts to work properly when fetchPolicy is set to 'no-cache'. Any update on this?

@UwUnyaa
Copy link

UwUnyaa commented Feb 27, 2019

Just bumped into this problem. It seems as if Apollo looks for a similar object in the cache and uses that without checking any variables. When cache persistence is enabled, even refreshing doesn't affect it, and the only way around it is to disable cache for the query, which is unacceptable when offline functionality is desired.

I'd assume the client making a new query when variables changed and data for them isn't in the cache is a sane default.

@mssngr
Copy link

mssngr commented Mar 6, 2019

Encountering this, myself. I'll just have to turn off the cache for now, which is a bummer of a workaround.

@adamjv90
Copy link

Also encountering this issue.

@dunika
Copy link

dunika commented Mar 20, 2019

I appreciate all the work done by the Apollo team but for me this is a very significant issue.

@TuHuynhVan
Copy link

I faced same issue and I put fetchPolicy: 'no-cache', that saved me after hours of debugging my React App.

        // Fetching new data for the PO Entity
        let pageObjectQueryResult = await client.query({
            query: getPageObjectEntityQuery,
            variables: {
                name: event.value
            },
            fetchPolicy: 'no-cache'
        });

@UwUnyaa
Copy link

UwUnyaa commented Mar 20, 2019

@TuHuynhVan that's a workaround which won't work for everyone. In my case, being able to cache queries is critical as I'm working on a PWA with offline functionality.

@hwillson
Copy link
Member

react-apollo 2.5.2 has a bug in it that prevents certain Query component updates from happening. This issue has been fixed in #2840, and will be published soon (hopefully tonight / tomorrow). I have a feeling this will help fix some of the issues people have been encountering here.

@jcready
Copy link

jcready commented Mar 20, 2019

@hwillson #2840 will not solve the problem.

@hwillson
Copy link
Member

@jcready It won't solve this entire issue thread, I agree. But I have a feeling it will help address some of the comments posted here in the past 2 weeks.

jcready added a commit to jcready/react-apollo that referenced this issue Mar 21, 2019
@IgorCRD
Copy link

IgorCRD commented Sep 19, 2019

@hwillson,
Version 3.1.0 seems to still have the same behavior.

To me we already have three solid approaches on how to solve this problem:

  1. Adding a clearPreviousData flag as a query argument;
  2. Adding a isPreviousData flag on the results object;
  3. Adding a shouldClearPreviousData function argument, as proposed by @jcready;

The first approach is my favorite because is more of a declarative/reacty way of doing things.

I guess the first two approaches would attend most of the use cases, but it does not invalidate the use cases solved by the third and I think after implementing one approach the other two would be trivial (guessing out of complete ignorance sorry if it is harder than it seems). The hooks version could have an array containing values that would cause the hook to clearPreviousData, it would work like the hooks version of the third approach.

Just brainstorming here to let you all know that this problem is still a thing and to get the discussion running again.

@markplindsay
Copy link

@hwillson, I am experiencing this problem when using useQuery in version 3.1.1 as a part of a Next.js app. I'm a new Apollo user and this is the first issue I've had with this excellent software.

  1. Component renders, and useQuery fetches results using variables {"currency": "INR"}
  2. Variables change to {"currency": "CAD"}
  3. useQuery correctly fetches and returns results using new variables
  4. Variables change back to {"currency": "INR"}
  5. useQuery incorrectly returns results as if variables were still {"currency": "CAD"}

Setting fetchPolicy: 'no-cache' as a useQuery option fixes it. Let me know if I can provide any more info.

@markplindsay
Copy link

@hwillson, my problem was actually the result of me not understanding how InMemoryCache works. There is no bug.

The {"currency": "INR"} in my comment above was actually part of a larger structure that looks like this:

{
  "search": [
    {
      "id": 321,
      "mpn": "NE555",
      "median_price_1000": {
        "currency": "INR",
        "price": 0.123,
        "__type": "PricePoint"
      },
      "__type": "Part"
    },
    {
      "id": 654,
      "mpn": "NE555P",
      "median_price_1000": {
        "currency": "INR",
        "price": 0.456,
        "__type": "PricePoint"
      },
      "__type": "Part"
    },
    {
      "id": 987,
      "mpn": "NE555R",
      "median_price_1000": {
        "currency": "INR",
        "price": 0.789,
        "__type": "PricePoint"
      },
      "__type": "Part"
    }
  ]
}

This over-simplified example was hiding the real cause of the problem. PricePoint doesn't have an ID, and so InMemoryCache was storing only the most-previously-fetched version of median_price_1000 for each Part as a part of its normalization process. It was treating all of a Part's PricePoint as being the same.

For anyone else who may be experiencing similar issues, by passing a dataIdFromObject function to InMemoryCache when initializing ApolloClient, I was able to append median_price_1000.currency to each Part's key to differentiate Part in the cache:

import withApollo from 'next-with-apollo'
import ApolloClient, { InMemoryCache, defaultDataIdFromObject } from 'apollo-boost'

export default withApollo(
  ({ initialState }) =>
    new ApolloClient({
      cache: new InMemoryCache({
        dataIdFromObject: (o: any) => {
          const id = defaultDataIdFromObject(o)
          if (o.__typename !== 'Part' || id === null) {
            return id
          }
          if (o.median_price_1000 === null) {
            return id
          }
          return `${id}:${o.median_price_1000.currency}`
        }
      }).restore(initialState || {}),
      ...
    })
)

This solved the caching problem without using fetchPolicy: 'no-cache'. There is probably an even better way to do this, but this works and is much better than disabling the cache entirely.

@UwUnyaa
Copy link

UwUnyaa commented Oct 1, 2019

@markplindsay The problem is that Apollo should warn you in development mode when your dataIdFromObject() returns wrong values that cause problems with serialization.

@macrozone
Copy link

macrozone commented Nov 5, 2019

@hwillson can confirm this happens with "@apollo/react-hooks@^3.1.3"

useQuery returns old data when variables change (with loading: true)

Edit: but maybe that is intentional and could make sense in some cases

@joshjg
Copy link

joshjg commented Nov 8, 2019

@hwillson
Currently useQuery returns variables alongside data - as a user I'd expect these variables to be those used to fetch the old data (while new data is loading). However, they seem to be the same variables as those in the options argument, which doesn't seem very useful.

@Daniel-Griffiths
Copy link

Same problem here, the data is always "one version" too old.

@psamim
Copy link

psamim commented Nov 27, 2019

Same here.

@afreix
Copy link

afreix commented Dec 12, 2019

Has anyone found a suitable workaround for this?

@declanelcocks
Copy link

@afreix the trick of adding a key to the component was one approach that worked for me. Put one of the variables as the key and it should trigger the querying component to re-render.

@sbcreates
Copy link

@declanelcocks could you elaborate?

I'm having the same issue as others. I have a context in my React app that, when it changes, the id variable I have in my Query is changed and a new query should be called followed by updating the data.

When my context changes to a new variable, a query if executed and new data is populated. If I change my context back to a previously fetched variable, the id variable in the query gets updated but the query is not executed.

If I change my fetchPolicy to no-cache the query works as expected and refetches data every time my context is updated.

@richardscarrott
Copy link

Yeah also experiencing this issue in 3.1.0 and 3.1.1.

@deresegetachew
Copy link

deresegetachew commented Feb 11, 2020

I am facing the same issue with useLazyQuery. My Scenario is I am using fetchMore to load more data and the fetchMoreResult , variables variables inside updateQuery of fetchMore are different from that off onCompleted.

The order of Execution is as follows:

FetchMore -> UpdateQuery({fetchMoreResult , variables}) -> then OnCompleted is Called.

variables and fetchMoreResult in UpdateQuery are up to date; but the data parameter in Completed is not updated.

FYI it doesnt matter if my fetch policy is fetchPolicy:'network-only',

@mhuconcern
Copy link

mhuconcern commented Feb 27, 2020

This still occurs on v2.6.8 for me which is a stable build. I think I have spent an honest 4 hours trying to fix this bug where

<Query 
variables={{this changes}}
fetchPolicy='network-only'/>
// Check network status, it's always 7
// Data is fulfilled twice, once with older param second with newer param, 
// causing intermittent ui flashes of data
</Query>

@trdaya
Copy link

trdaya commented Mar 2, 2020

This still occurs on v2.6.8 for me which is a stable build. I think I have spent an honest 4 hours trying to fix this bug where

<Query 
variables={{this changes}}
fetchPolicy='network-only'/>
// Check network status, it's always 7
// Data is fulfilled twice, once with older param second with newer param, 
// causing intermittent ui flashes of data
</Query>

@mhuconcern I am facing the exact same issue with the version you mentioned

@mhuconcern
Copy link

mhuconcern commented Mar 3, 2020

This still occurs on v2.6.8 for me which is a stable build. I think I have spent an honest 4 hours trying to fix this bug where

<Query 
variables={{this changes}}
fetchPolicy='network-only'/>
// Check network status, it's always 7
// Data is fulfilled twice, once with older param second with newer param, 
// causing intermittent ui flashes of data
</Query>

@mhuconcern I am facing the exact same issue with the version you mentioned

@trdaya #321 (comment) . If this helps you. I realize that this bug even though it exists on this build, it is exposed more if your graphql apis don't have a way to define if the returned object is unique to the request param

@trdaya
Copy link

trdaya commented Mar 17, 2020

This still occurs on v2.6.8 for me which is a stable build. I think I have spent an honest 4 hours trying to fix this bug where

<Query 
variables={{this changes}}
fetchPolicy='network-only'/>
// Check network status, it's always 7
// Data is fulfilled twice, once with older param second with newer param, 
// causing intermittent ui flashes of data
</Query>

@mhuconcern I am facing the exact same issue with the version you mentioned

@trdaya #321 (comment) . If this helps you. I realize that this bug even though it exists on this build, it is exposed more if your graphql apis don't have a way to define if the returned object is unique to the request param

@mhuconcern thanks 👍 . I tried this but unfortunately, it didn't work for me.
Some assumptions and findings:
Since the fetchPolicy is cache-and-network:

  1. It returns the data (if any) from the cache. networkStatus is 7 if the data (stale data) is already present in the cache
  2. It makes a network call in parallel to step 1, to fetch the latest data, to keep the cache up to date
  3. When the network call completes and the cache is updated, new data is pushed to the client. networkStatus 7 (this time because data fetching from network is completed) with new data.

I tried throttling my network on chrome's dev-tools and found that this issue doesn't happen on slow networks. networkStatus 7 is received only once, with the new data.

@WillSquire
Copy link

Just incase anyone else is having issues working around this, comparing current data to previousResult in updateQuery works for me. E.g.:

return fetchMoreResult.length === 0 || data !== previousResult
          ? previousResult
          : // add new data

@sarojkumar007
Copy link

This is a pretty common issue. You need to do a check for equality of a name/id and show a loading component or pass an empty/default obj if you don’t want to see stale data. I would advise modifying the data variable, if that’s the same one from the query result obj. Just use a ternary or if/else. You may need to abstract it to an external component to ensure you get rerenders reliably.

On Wed, Jul 25, 2018 at 10:45 AM James Wyatt Cready-Pyle < @.***> wrote: Yeah, you basically have to use something like this to prevent rendering data from a query with different variables: if (data.reddit && data.reddit.subreddit.name !== this.state.search) { data = {}; } And that only works if the data being returned also includes the variable you passed in. Definitely seems like a bug. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2202 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AIM6lxQm220hAw84gsU2XQj043KYvC17ks5uKKCzgaJpZM4VZucg .

Worked for me. Thanks

@krvajal
Copy link

krvajal commented Apr 2, 2020

In my case I am doing the following

  let { data, loading, fetchMore, networkStatus } = useQuery<GetCompanyLeadsQuery, GetCompanyLeadsQueryVariables>(GET_COMPANY_LEADS_QUERY, {
        variables: {
            type: ContactType.Vendor,
            query: queryField.value,
            page: 1,
        },
        notifyOnNetworkStatusChange: true,
    });

    if(networkStatus === NetworkStatus.setVariables) {
        data = undefined;
    }

So when the query variable changes, the network status is set to setVariables and I set data to undefined to avoid rendering stale result for another query.

@seeruk
Copy link

seeruk commented Apr 24, 2020

@krvajal As far as I can see, the problem there is that if you do have the data in the cache you end up wiping it out then with that approach.

The only approach I've currently seen working is somehow echoing some of the data included in your query variables in the data so you can see if the data is new or not.

This is a huge issue for applications that want to show up-to-date data, but still respond extremely quickly and opportunistically, and I've not seen a good solution that doesn't rely on using the response data (which isn't always possible).

@lloydaf
Copy link

lloydaf commented Apr 24, 2020

This is proving to be such a big pain. Trying to fit in rxjs into the mix is fun(not). After spending a lot of time working with Angular, it's amusing to me that useLazyQuery doesn't even return a promise at this point. Maybe it's just me.

@Elijen
Copy link

Elijen commented May 24, 2020

I am considering leaving development job and starting to work as a bus driver because of this bug.

@lukaskl

This comment has been minimized.

@codefreak13
Copy link

I was able to solve this by using a combination of client.resetStore() on my logout function and fetchPolicy='network-only' on my Query.

@Javey
Copy link

Javey commented Jul 14, 2020

I use @client directive to set a local value, when I have mutated it, Apollo will update it in next Microtask, so I read the previous result.

new QueryData({
    options: updatedOptions,
    context: context,
    onNewData: function () {
        if (!queryData.ssrInitiated()) {
            // update next tick
            Promise.resolve().then(forceUpdate);
        }
        else {
            forceUpdate();
        }
    },
});``

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository
Projects
None yet
Development

Successfully merging a pull request may close this issue.