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

[PLAT-9731] Fix XHR delivery postReportCallback #1938

Merged
merged 8 commits into from
Mar 13, 2023

Conversation

gingerbenw
Copy link
Member

Goal

To ensure delivery errors are passed to the post report callback when using the delivery-xml-http-request plugin

Design

Follow the same implementation as delivery-x-domain-request

Testing

Added new unit test and update existing test to verify errors are handled correctly

@github-actions
Copy link

github-actions bot commented Mar 9, 2023

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 43.45 kB 13.36 kB
After 43.63 kB 13.37 kB
± ⚠️ +181 bytes ⚠️ +13 bytes

code coverage diff

<temporarily disabled>

Generated by 🚫 dangerJS against 785ef64

@gingerbenw gingerbenw marked this pull request as ready for review March 9, 2023 13:37
packages/delivery-xml-http-request/delivery.js Outdated Show resolved Hide resolved
packages/delivery-xml-http-request/delivery.js Outdated Show resolved Hide resolved
@gingerbenw
Copy link
Member Author

latest changes to the xhr mocks are a bit more sweeping, but a lot more similar to the real thing

@@ -11,14 +11,18 @@ module.exports = (client, win = window) => ({
if (req.readyState === win.XMLHttpRequest.DONE) {
const status = req.status
if (status === 0 || status >= 400) {
client._logger.error('Event failed to send…')
const err = new Error(`Request failed with status ${status}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth applying this to sendSession as well — it doesn't look like we currently use/expose the cb parameter there but if that changes in the future then it'd be nice for it to not have the same bug

@gingerbenw gingerbenw merged commit 2f1eee7 into next Mar 13, 2023
@yousif-bugsnag yousif-bugsnag mentioned this pull request Apr 25, 2023
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.

2 participants