-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(utils): Handle when requests get aborted in fetch instrumentation #13202
Conversation
size-limit report 📦
|
f260a36
to
f9e328c
Compare
f9e328c
to
7da58d1
Compare
okay I think the e2e test covers it, but we probably also want some tests in https://github.com/getsentry/sentry-javascript/tree/develop/dev-packages/browser-integration-tests to validate in firefox and safari as well. |
...s/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts
Outdated
Show resolved
Hide resolved
...s/browser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts
Outdated
Show resolved
Hide resolved
// NOTE: If you are a Sentry user, and you are seeing this stack frame, | ||
// it means the sentry.javascript SDK caught an error invoking your application code. | ||
// This is expected behavior and NOT indicative of a bug with sentry.javascript. | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lforst this is a breaking change for apps that do not handle fetch exceptions :( as they did not get the exceptions before and now they do....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate? Ideally in a new issue. I do not think this alters behaviour. This actually fixes a bug where we swallowed errors.
resolves #13187
WIP as we get better test cases