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 XHR Transport #4803

Merged
merged 7 commits into from
Mar 29, 2022
Merged

feat(browser): Add new v7 XHR Transport #4803

merged 7 commits into from
Mar 29, 2022

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Mar 28, 2022

This PR adds 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. Note that I didn't include integration tests for this change as I did not yet find a way to "deactivate" the Fetch API in our playwright test framework. If I figured out a way to do this, Integration tests will follow in a separate PR.

ref: https://getsentry.atlassian.net/browse/WEB-727

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.09 KB (+0.4% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 64.55 KB (+0.9% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.78 KB (+0.68% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 57.96 KB (+0.99% 🔺)
@sentry/browser - Webpack (gzipped + minified) 23.22 KB (+0.35% 🔺)
@sentry/browser - Webpack (minified) 82.47 KB (+0.68% 🔺)
@sentry/react - Webpack (gzipped + minified) 23.26 KB (+0.34% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.2 KB (+0.18% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.01 KB (+0.4% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.42 KB (+0.52% 🔺)

@Lms24 Lms24 force-pushed the lms-new-xhr-transport branch from db1dcd4 to 57a06f4 Compare March 29, 2022 08:29
@Lms24 Lms24 marked this pull request as ready for review March 29, 2022 11:14
@Lms24
Copy link
Member Author

Lms24 commented Mar 29, 2022

CI should pass once #4810 is done and merged (the errors seem to be related to MongoDB)

@Lms24 Lms24 requested review from AbhiPrasad and lforst March 29, 2022 11:16
const XHR_READYSTATE_DONE = 4;

export interface XHRTransportOptions extends BaseTransportOptions {
headers?: { [key: string]: string };
Copy link
Member Author

@Lms24 Lms24 Mar 29, 2022

Choose a reason for hiding this comment

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

Decided to go with {[key: string]: string} instead of Record as it is used that way in TransportOptions

export interface TransportOptions {
/** Sentry DSN */
dsn: DsnLike;
/** Define custom headers */
headers?: { [key: string]: string };

which are passed to makeNewXHRTransport. Seems to be more consistent that way IMHO

@Lms24 Lms24 force-pushed the lms-new-xhr-transport branch from 1224b1d to 4672d15 Compare March 29, 2022 12:14
@Lms24 Lms24 merged commit fa58281 into master Mar 29, 2022
@Lms24 Lms24 deleted the lms-new-xhr-transport branch March 29, 2022 12:38
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
* 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.

2 participants