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

Log MissingFieldErrors in ObservableQuery#getCurrentResult rather than using result.error #8604

Merged

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Aug 6, 2021

Should fix #8593, which suggests our decision in #8262 to include MissingFieldErrors in result.error (beginning in @apollo/client@3.4.0-rc.1) was probably more disruptive than helpful.

Copy link

@hueter hueter left a comment

Choose a reason for hiding this comment

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

This would help my team's issues a lot, thank you for the quick turnaround!

src/core/ObservableQuery.ts Show resolved Hide resolved
benjamn added a commit that referenced this pull request Aug 6, 2021
@benjamn benjamn force-pushed the issue-8593-stop-reporting-MissingFieldErrors-as-result.error branch from 3502c1a to 50e0413 Compare August 6, 2021 20:56
@tvler
Copy link

tvler commented Aug 8, 2021

is there a way to suppress this manually for the time being until this is shipped?

@brainkim
Copy link
Contributor

brainkim commented Aug 9, 2021

Two non-blocking questions:

  1. How does this affect users who are now expecting one of data, error, loading to be defined? (Query data and error are undefined #8063)
  2. What is the root cause of these transitory missing field errors, regardless of whether or not they’re logged or added to the result?

@benjamn
Copy link
Member Author

benjamn commented Aug 9, 2021

@brainkim

  1. We should keep looking for ways to prevent or explain situations where data, error, and/or loading have nonsensical/inconsistent values, but providing missing field errors via result.error (which I know I supported for in pass along missing field errors to the user #8262) was not the answer. This PR contributes to the explainability story by making it possible to recover the hidden errors by calling setLogVerbosity("debug"), but I don't expect this to be our last attempt to solve this problem.

  2. Missing field errors are natural and expected in the course of using InMemoryCache. The cache doesn't start out with anything in it, yet we read from the empty cache in many normal situations, and that produces missing field errors. The errors become problematic when you're reading data that you expected to exist, which can happen due to cache eviction (for example) or lists populated by different queries with incompatible selection sets. Unfortunately, I don't know how to tell the difference between what was expected versus what was surprising, so I think we need to prioritize not erroring when nothing's actually wrong, so people can update from v3.3.x to v3.4.x without worrying about this new source of errors.

Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

LGTM

@benjamn benjamn force-pushed the issue-8593-stop-reporting-MissingFieldErrors-as-result.error branch from 6091a3e to 1e8f677 Compare August 9, 2021 15:28
@benjamn benjamn merged commit 8f25d2a into main Aug 9, 2021
@benjamn benjamn deleted the issue-8593-stop-reporting-MissingFieldErrors-as-result.error branch August 9, 2021 15:34
@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.

Can't find field on ROOT_QUERY object error on 3.4.x and up
4 participants