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

Stop delivering previous data in unrelated loading results. #6566

Merged
merged 2 commits into from
Jul 9, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jul 9, 2020

Ever since apollographql/react-apollo#1639, Apollo's React functionality has preserved a strange and nonsensical policy of falling back to the previous data for loading: true results, even though the previous data may have nothing to do with the current request, as #6039 demonstrates. Results with loading: true may provide data from the cache (if any), but they should never simply reuse data from a previous result, which may have been derived using different variables. Instead, result.data should be undefined for loading states that do not have any partial cache data to provide.

This is potentially a breaking change for code that blindly relied on result.data being defined whenever result.loading is true, but I believe the bugs this change will fix are much more serious than the inconvenience of checking the truthiness of result.data before using it. Still, I wish I'd gotten to this sooner.

Fixes #6039, as verified by the reproduction provided by @davismariotti (thanks!).

Results with loading:true can still provide data from the cache, but they
should never provide data from a previous result, especially since the
previous result may have been derived from entirely different variables:
#6039 (comment)

This is potentially a breaking change for code that relied on result.data
not being undefined when result.loading is true, but the bugs that this
change will fix are much more serious than the inconvenience of checking
the truthiness of result.data before using it.

Fixes #6039, as verified by the reproduction provided by @davismariotti:
https://codesandbox.io/s/changing-variables-demo-obykj
Comment on lines 371 to +372
if (loading) {
const previousData =
this.previousData.result && this.previousData.result.data;
result.data =
previousData && data
? {
...previousData,
...data
}
: previousData || data;
// Fall through without modifying result...
Copy link
Member Author

@benjamn benjamn Jul 9, 2020

Choose a reason for hiding this comment

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

I suppose we could default result.data to an empty object here, if we think that will help reduce the risk of this (admittedly late-breaking) change. However, that logic would only apply for React code using QueryData class.

Copy link
Member

Choose a reason for hiding this comment

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

Storing empty objects in data has gotten us into trouble in the past (as outlined in apollographql/react-apollo#3388), so I'm all for keeping this as is.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Looks great @benjamn! (I'm surprised / happy so few tests needed to change for this 🎉)

Comment on lines 371 to +372
if (loading) {
const previousData =
this.previousData.result && this.previousData.result.data;
result.data =
previousData && data
? {
...previousData,
...data
}
: previousData || data;
// Fall through without modifying result...
Copy link
Member

Choose a reason for hiding this comment

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

Storing empty objects in data has gotten us into trouble in the past (as outlined in apollographql/react-apollo#3388), so I'm all for keeping this as is.

@jfrolich
Copy link

jfrolich commented Jul 15, 2020

Might be nonsensical, but it's actually very useful in user interfaces with a filter UI. You'd almost never want to completely go back to a global spinner when a filter is changed. We use this a lot in our app, and it's completely broken after this release. Is this coming back as an option?

I don't think it is unreasonable to do it explicitly in user code. But this would be the largest breaking change that probably nobody expects in Apollo Client 3. So it would be very important to highlight very largely in the release notes.

@macrozone
Copy link

@jfrolich absolutly agree! I was blaming all the breakings on the new cache, but a lot of issues was due to this change and its not mentioned as breaking change (or i did not see it)

@jfrolich as a workaround you can do:

// store previous data in a ref
 const previousData = useRef<AssortmentFilters["search"]>();
// your query
  const { data, error, loading } = useQuery<
    AssortmentFilters,
    AssortmentFiltersVariables
  >(QUERY, {
    variables: { assortmentId, searchQueryString, filterQuery },
    skip: skip || (!assortmentId && !searchQueryString),
  });

  if (data?.search) {
    previousData.current = data.search;
  }
  const filters = (data?.search ?? previousData.current)?.filters ?? [];

  return { filters, error, loading };

@maapteh
Copy link

maapteh commented Jul 17, 2020

For our application this was a big surprise, for the same reason above. Given new arguments to same query now results in total empty data :( Its oke to make the software better, but please mention it in the migration doc.

@robertsmit
Copy link

Is it possible to get the old behavior. For example when typing in a searchField en changing the searchText, it is ok to get the old results for the possible options....

@kliakos
Copy link

kliakos commented Sep 3, 2020

Would be nice to optionally have the old behavior back somehow.

@eranimo
Copy link

eranimo commented Sep 15, 2020

This should have been disabled by default instead of removed entirely. This is a huge breaking change that is not easy to fix. This upgrade is turning out to be quite the headache.

@akornato
Copy link

In an async world it is "nonsensical" to think a user never wants to see old data while they wait for new data. I'm sure there's exceptions, but that's what loading is for. Fortunately we wrap useQuery anyway, thanks @macrozone for the hint how to work around this new behaviour.

@hwillson
Copy link
Member

Just a heads up, previousData is now available alongside data in @apollo/client@next (see #7082). 3.3 will be officially published early next week.

@ctataryn
Copy link
Contributor

@hwillson FYI I have added this PR to update the v2 -> v3 migration guide, as this breaking change was a challenge to track down.

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

Successfully merging this pull request may close these issues.

When variables change, data is the previous result in Query Component
10 participants