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

feat(browser): Add new v7 Fetch Transport #4765

Merged
merged 13 commits into from
Mar 28, 2022
Merged

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Mar 23, 2022

This PR creates the new v7 Fetch Transport, and updates the browser backend to use the new transport. To configure the transport, users can supply requestOptions, which is supplied to the fetch request. This consolidates the earlier pattern of passing in both headers and fetchParameters that the old fetch transport used to use.

Part of #4660

Resolves https://getsentry.atlassian.net/browse/WEB-726

// 2. Restore the original version of the request body, which is commented out
// 3. Search for either of the PR URLs above and pull out the companion hacks in the browser playwright tests and the
// baseClient tests in this package
enhanceEventWithSdkInfo(event, api.metadata.sdk);
Copy link
Member Author

Choose a reason for hiding this comment

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

not nice to duplicate all of this, but considering we are deleting stuff very soon, I think duplicating this code is fine.

@AbhiPrasad AbhiPrasad marked this pull request as ready for review March 24, 2022 14:13
@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.01 KB (+1.81% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 63.98 KB (+1.98% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.66 KB (+1.82% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 57.39 KB (+2.01% 🔺)
@sentry/browser - Webpack (gzipped + minified) 23.14 KB (+1.5% 🔺)
@sentry/browser - Webpack (minified) 81.92 KB (+1.53% 🔺)
@sentry/react - Webpack (gzipped + minified) 23.18 KB (+1.51% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.12 KB (+0.72% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.91 KB (+1.45% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.29 KB (+1.51% 🔺)

@AbhiPrasad
Copy link
Member Author

ouch size-limit bot

@AbhiPrasad
Copy link
Member Author

Opened #4778 with some changes from this PR

@AbhiPrasad
Copy link
Member Author

So it seems this is failing on the minified bundles 🤔

@AbhiPrasad
Copy link
Member Author

So in regular bundles it works fine (bundle_es6,bundle_es5) , but in minified bundles (like bundle_es5_min) I get a CORS error

Access to fetch at 'https://dsn.ingest.sentry.io/api/1337/store/?sentry_key=public&sentry_version=7' from origin 'null' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.
Failed to load resource: net::ERR_FAILED

@onurtemizkan any idea why this might be happening?

@onurtemizkan
Copy link
Collaborator

onurtemizkan commented Mar 28, 2022

@onurtemizkan any idea why this might be happening?

@AbhiPrasad, that's very strange, there aren't any special cases for minified bundles, while building pages.

@AbhiPrasad
Copy link
Member Author

Figured it out, it's because the minified bundle minifies the _experiments option! Testing on multiple bundles caught this!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Aside from the _experiments problem (and the linter check), these changes look good to me. Also made a lot of sense when going over them while implementing #4803.

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) March 28, 2022 19:09
@AbhiPrasad AbhiPrasad merged commit 249302a into master Mar 28, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-new-fetch-transport branch March 28, 2022 19:45
Lms24 added a commit that referenced this pull request Mar 29, 2022
* add the new XHR Transport creation functionality. The function makeNewXHRTransport(...) creates a Transport that is based on the browser's XMLHttpRequest API. It is used as a fallback if the Fetch API is not available (IE11...). The creation function is similar to the new Fetch Transport creation introduced in #4765.

* in addition to the transport creation function, this PR also adds tests which verify the correct calls to the XMLHttpRequest API. Furthermore, the tests check for correct request/response header setting.
lforst pushed a commit that referenced this pull request Mar 29, 2022
* add the new XHR Transport creation functionality. The function makeNewXHRTransport(...) creates a Transport that is based on the browser's XMLHttpRequest API. It is used as a fallback if the Fetch API is not available (IE11...). The creation function is similar to the new Fetch Transport creation introduced in #4765.

* in addition to the transport creation function, this PR also adds tests which verify the correct calls to the XMLHttpRequest API. Furthermore, the tests check for correct request/response header setting.
lforst pushed a commit that referenced this pull request Mar 29, 2022
This PR creates the new v7 Fetch Transport, and updates the browser backend to use the new transport. To configure the transport, users can supply `requestOptions`, which is supplied to the fetch request. This consolidates the earlier pattern of passing in both headers and fetchParameters that the old fetch transport used to use.
lforst pushed a commit that referenced this pull request Mar 29, 2022
* add the new XHR Transport creation functionality. The function makeNewXHRTransport(...) creates a Transport that is based on the browser's XMLHttpRequest API. It is used as a fallback if the Fetch API is not available (IE11...). The creation function is similar to the new Fetch Transport creation introduced in #4765.

* in addition to the transport creation function, this PR also adds tests which verify the correct calls to the XMLHttpRequest API. Furthermore, the tests check for correct request/response header setting.
Lms24 added a commit that referenced this pull request Mar 29, 2022
add integration tests to test the new XHR transport introduced in #4803

* the tests are very similar to the new Fetch transport integration tests introduced in #4765. The only difference is that they disable the browsers' Fetch API by setting window.fetch = undefined. This way, the SDK falls back to the XHR transport.
@AbhiPrasad AbhiPrasad added this to the Pre 7.0.0 Work milestone Apr 6, 2022
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.

3 participants