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

(core) - fix incremental fetch not bubbling up JS-errors #2210

Merged
merged 4 commits into from
Jan 21, 2022

Conversation

JoviDeCroock
Copy link
Collaborator

@JoviDeCroock JoviDeCroock commented Jan 21, 2022

Summary

When we introduced incremental fetch we made sure that the backwards exchanges flow could be triggered multiple times for an OperationResult this made it so that the fetchSource would actually have the backwards execution of the exchanges like GraphCache writing data, ... inside of the try catch which means that if one of these throws we end up in the catch and handle this as if we encountered a GraphQL error.

fixes #2191

Set of changes

  • filter out Syntax, Type and normal Error scenario's coming from the reverse-exchange run

@JoviDeCroock JoviDeCroock requested a review from kitten January 21, 2022 16:48
@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2022

🦋 Changeset detected

Latest commit: 8328ef6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@urql/core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -146,6 +146,14 @@ export const makeFetchSource = (
})
.then(complete)
.catch((error: Error) => {
if (error.name === 'SyntaxError' || error.name === 'TypeError') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might be a bit too limiting 😅

Copy link
Member

Choose a reason for hiding this comment

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

Should we limit this to development?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well my thinking was that people using for instance Sentry in production would want to see these errors surface as well

Copy link
Member

Choose a reason for hiding this comment

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

oh yea good point

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

Successfully merging this pull request may close these issues.

Cache update errors are caught
2 participants