Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(utils): Handle when requests get aborted in fetch instrumentation #13202

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

AbhiPrasad
Copy link
Member

resolves #13187

WIP as we get better test cases

@AbhiPrasad AbhiPrasad changed the title fix(utils): Handle when requests get aborted fix(utils): Handle when requests get aborted in fetch instrumentation Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

size-limit report 📦

Path Size
@sentry/browser 22.46 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing) 34.24 KB (+0.01% 🔺)
@sentry/browser (incl. Tracing, Replay) 70.29 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.63 KB (-0.01% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) 74.69 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 87.29 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 89.13 KB (0%)
@sentry/browser (incl. metrics) 26.76 KB (-0.02% 🔽)
@sentry/browser (incl. Feedback) 39.39 KB (-0.02% 🔽)
@sentry/browser (incl. sendFeedback) 27.08 KB (-0.02% 🔽)
@sentry/browser (incl. FeedbackAsync) 31.72 KB (-0.02% 🔽)
@sentry/react 25.22 KB (-0.04% 🔽)
@sentry/react (incl. Tracing) 37.23 KB (-0.02% 🔽)
@sentry/vue 26.6 KB (-0.03% 🔽)
@sentry/vue (incl. Tracing) 36.08 KB (-0.01% 🔽)
@sentry/svelte 22.59 KB (-0.03% 🔽)
CDN Bundle 23.65 KB (-0.03% 🔽)
CDN Bundle (incl. Tracing) 35.89 KB (-0.01% 🔽)
CDN Bundle (incl. Tracing, Replay) 70.32 KB (-0.01% 🔽)
CDN Bundle (incl. Tracing, Replay, Feedback) 75.59 KB (-0.01% 🔽)
CDN Bundle - uncompressed 69.4 KB (-0.01% 🔽)
CDN Bundle (incl. Tracing) - uncompressed 106.32 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 218.17 KB (+0.01% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 231.08 KB (+0.01% 🔺)
@sentry/nextjs (client) 37.08 KB (-0.02% 🔽)
@sentry/sveltekit (client) 34.81 KB (-0.01% 🔽)
@sentry/node 114.77 KB (+0.01% 🔺)
@sentry/node - without tracing 89.33 KB (+0.01% 🔺)
@sentry/aws-serverless 98.5 KB (+0.01% 🔺)

@AbhiPrasad AbhiPrasad force-pushed the abhi-fetch-abort-controller branch 3 times, most recently from f260a36 to f9e328c Compare August 2, 2024 17:14
@AbhiPrasad AbhiPrasad force-pushed the abhi-fetch-abort-controller branch from f9e328c to 7da58d1 Compare August 2, 2024 21:00
@AbhiPrasad
Copy link
Member Author

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.

@chargome chargome marked this pull request as ready for review August 5, 2024 09:09
@chargome chargome requested a review from lforst August 5, 2024 09:09
@lforst lforst merged commit 957324e into develop Aug 5, 2024
125 checks passed
@lforst lforst deleted the abhi-fetch-abort-controller branch August 5, 2024 11:03
// 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;
Copy link

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....

Copy link
Member

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.

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

Successfully merging this pull request may close these issues.

Fetch resolves before response has been received; response is undefined
4 participants