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

includeError displaying errors as "[Array]" #278

Closed
alesso-x opened this issue Feb 15, 2021 · 10 comments · Fixed by #279
Closed

includeError displaying errors as "[Array]" #278

alesso-x opened this issue Feb 15, 2021 · 10 comments · Fixed by #279

Comments

@alesso-x
Copy link

alesso-x commented Feb 15, 2021

I'm having an issue where the errors are shown as "[Array]", is there a way to map this appropriately? I had to add my own breadcrumb with errorLink. Thank you in advance, great package so far!

Screen Shot 2021-02-15 at 6 40 47 PM

@DiederikvandenB
Copy link
Owner

Hi, since we recently went through a breaking update, it would be helpful to know which version of the package you're using.

@DiederikvandenB
Copy link
Owner

@spawnia it seems like you removed the following code in the rewrite:
https://github.com/DiederikvandenB/apollo-link-sentry/blob/v2.1.0/src/utils.ts#L13-L15

I think adding this back would resolve the issue. At the very least, it will format the error better.

@spawnia
Copy link
Collaborator

spawnia commented Feb 16, 2021

The operation name and type indicate this is definitely version 3. I will begin working on a PR to restore nice serialization of errors right now.

@spawnia
Copy link
Collaborator

spawnia commented Feb 16, 2021

Serialization of fetchResult seems to be broken in the same way:

image

@alesso-x
Copy link
Author

alesso-x commented Feb 18, 2021

Thank you @DiederikvandenB @spawnia, I can now see the errors formatted correctly!

Screen Shot 2021-02-18 at 2 05 50 PM

I have a follow up question and it might be a feature request. I can see the error nicely now but if I wanted to see the results I would have to enable includeFetchResult. That config changes all request to show the fetch result but I only want to see it for errors. Could we have a config var to only show fetch results for errors? Or is there another way I could do this on the client side with the transform function? I could also submit a PR if that makes easier. Thank you again!

Screen Shot 2021-02-18 at 2 09 32 PM

@spawnia
Copy link
Collaborator

spawnia commented Feb 18, 2021

How about includeFetchResult: 'on-error' as a third option?

@alesso-x
Copy link
Author

How about includeFetchResult: 'on-error' as a third option?

I like that idea

@spawnia
Copy link
Collaborator

spawnia commented Feb 18, 2021

How does Apollo Client behave when it receives a 200 that also has errors, will it call .error() or .success() on the subscriber? And how should we handle that? I think there is no hard and fast rule to know if those errors are something actionable.

@alesso-x
Copy link
Author

alesso-x commented Feb 22, 2021

How does Apollo Client behave when it receives a 200 that also has errors, will it call .error() or .success() on the subscriber

I'm not sure, I would need to test it out. For now I'm able to see graphQL errors by adding a breadcrumb in a errorLink. If we could follow that, I think that would work for me.

Screen Shot 2021-02-22 at 11 10 46 AM

Also I had to disable transaction name in the plugin because it would override every error. For example I threw an error in a button and it would set the operation name to the query when the query was successful (the second issue is what it should look like). I think it makes sense to scope many of these options to errors. I have includeQuery enabled for every request.

Screen Shot 2021-02-18 at 9 11 30 PM

For completeness, here is my config for SentryLink. So far this setup is working for me but it would be nice to have all the config in SentryLink instead of a custom errorLink.

const sentryLink = new SentryLink({
  setFingerprint: false,
  setTransaction: false,
  attachBreadcrumbs: {
    includeQuery: true,
    includeError: true,
  },
})

@spawnia
Copy link
Collaborator

spawnia commented Feb 22, 2021

@alesso-x you have some valid points, but could you open discrete issues? I think that would be more productive than us discussing in this closed issue 😉

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 a pull request may close this issue.

3 participants