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

fix: Prevent fetch errors loops with invalid fetch implementations #3318

Merged
merged 1 commit into from Mar 10, 2021

Conversation

kamilogorek
Copy link
Contributor

No description provided.

@kamilogorek kamilogorek requested a review from HazAT March 10, 2021 08:59
@kamilogorek kamilogorek force-pushed the fetch-loop-two branch 3 times, most recently from c88d351 to a0b2f8c Compare March 10, 2021 09:12
@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.43 KB (+0.72% 🔺)
@sentry/browser - Webpack 21.28 KB (+0.4% 🔺)
@sentry/react - Webpack 21.32 KB (+0.39% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.51 KB (+0.54% 🔺)

Copy link
Member

@mitsuhiko mitsuhiko left a comment

Choose a reason for hiding this comment

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

Approved but we should validate if this can cause issues with csp.

@kamilogorek kamilogorek merged commit ddda018 into master Mar 10, 2021
@kamilogorek kamilogorek deleted the fetch-loop-two branch March 10, 2021 15:59
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
Comment on lines +54 to +60
const sandbox = document.createElement('iframe');
sandbox.hidden = true;
document.head.appendChild(sandbox);
if (sandbox.contentWindow?.fetch) {
return sandbox.contentWindow.fetch.bind(global);
}
document.head.removeChild(sandbox);
Copy link
Contributor

@sod sod Mar 19, 2021

Choose a reason for hiding this comment

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

This is so slow, that it doesn't execute below 50ms for mobile lighthouse. Thus it delays the TTI metric plus adds to TBT in lighthouse, even if you only execute install in a single macrotask, thus including sentry now costs you points in mobile lighthouse and lowers your ranking on google.

desktop

image

mobile

image

Copy link
Contributor

Choose a reason for hiding this comment

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

This was 6.0.1 btw., so quite some regression

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting us know, should be patched in #3341

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.

None yet

3 participants