From 9f204194b4e6b113aa5b7a93d7e02cbda8eb4d16 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 24 Mar 2022 18:43:23 +0100 Subject: [PATCH 1/7] feat(browser) add new v7 xhr transport add makeNewXHRTransport function (WIP, needs testing( --- packages/browser/src/transports/new-xhr.ts | 50 ++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 packages/browser/src/transports/new-xhr.ts diff --git a/packages/browser/src/transports/new-xhr.ts b/packages/browser/src/transports/new-xhr.ts new file mode 100644 index 000000000000..1dc0b61e69d1 --- /dev/null +++ b/packages/browser/src/transports/new-xhr.ts @@ -0,0 +1,50 @@ +import { + BaseTransportOptions, + createTransport, + NewTransport, + TransportMakeRequestResponse, + TransportRequest, +} from '@sentry/core'; +import { SyncPromise } from '@sentry/utils'; + +export interface XHRTransportOptions extends BaseTransportOptions { + headers?: { [key: string]: string }; +} + +/** + * Creates a Transport that uses the XMLHttpRequest API to send events to Sentry. + */ +export function makeNewXHRTransport(options: XHRTransportOptions): NewTransport { + function makeRequest(request: TransportRequest): PromiseLike { + return new SyncPromise((resolve, _reject) => { + const xhr = new XMLHttpRequest(); + + xhr.onreadystatechange = (): void => { + if (xhr.readyState === 4) { + const headers = { + 'x-sentry-rate-limits': xhr.getResponseHeader('X-Sentry-Rate-Limits'), + 'retry-after': xhr.getResponseHeader('Retry-After'), + }; + const response = { + body: xhr.response, + headers, + reason: xhr.statusText, + statusCode: xhr.status, + }; + + resolve(response); + } + }; + + xhr.open('POST', options.url); + for (const header in options.headers) { + if (Object.prototype.hasOwnProperty.call(options.headers, header)) { + xhr.setRequestHeader(header, options.headers[header]); + } + } + xhr.send(request.body); + }); + } + + return createTransport({ bufferSize: options.bufferSize }, makeRequest); +} From b28aae16dbfac106b0071e061c7da62a227f6b4c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 25 Mar 2022 14:59:38 +0100 Subject: [PATCH 2/7] add first test --- packages/browser/src/transports/index.ts | 1 + packages/browser/src/transports/new-xhr.ts | 12 ++-- .../test/unit/transports/new-xhr.test.ts | 65 +++++++++++++++++++ 3 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 packages/browser/test/unit/transports/new-xhr.test.ts diff --git a/packages/browser/src/transports/index.ts b/packages/browser/src/transports/index.ts index 8f938399e253..287e14e0ac50 100644 --- a/packages/browser/src/transports/index.ts +++ b/packages/browser/src/transports/index.ts @@ -3,3 +3,4 @@ export { FetchTransport } from './fetch'; export { XHRTransport } from './xhr'; export { makeNewFetchTransport } from './new-fetch'; +export { makeNewXHRTransport } from './new-xhr'; diff --git a/packages/browser/src/transports/new-xhr.ts b/packages/browser/src/transports/new-xhr.ts index 1dc0b61e69d1..6a8c6c4ae625 100644 --- a/packages/browser/src/transports/new-xhr.ts +++ b/packages/browser/src/transports/new-xhr.ts @@ -8,7 +8,9 @@ import { import { SyncPromise } from '@sentry/utils'; export interface XHRTransportOptions extends BaseTransportOptions { + // TODO choose whatever is preferred here (I like record more for easier readability) headers?: { [key: string]: string }; + // headers?: Record; } /** @@ -21,17 +23,17 @@ export function makeNewXHRTransport(options: XHRTransportOptions): NewTransport xhr.onreadystatechange = (): void => { if (xhr.readyState === 4) { - const headers = { - 'x-sentry-rate-limits': xhr.getResponseHeader('X-Sentry-Rate-Limits'), - 'retry-after': xhr.getResponseHeader('Retry-After'), - }; const response = { body: xhr.response, - headers, + headers: { + 'x-sentry-rate-limits': xhr.getResponseHeader('X-Sentry-Rate-Limits'), + 'retry-after': xhr.getResponseHeader('Retry-After'), + }, reason: xhr.statusText, statusCode: xhr.status, }; + // TODO when to reject? is it necessary at all, given that createTransport handles rejections? resolve(response); } }; diff --git a/packages/browser/test/unit/transports/new-xhr.test.ts b/packages/browser/test/unit/transports/new-xhr.test.ts new file mode 100644 index 000000000000..a99e38c1ce4f --- /dev/null +++ b/packages/browser/test/unit/transports/new-xhr.test.ts @@ -0,0 +1,65 @@ +import { EventEnvelope, EventItem } from '@sentry/types'; +import { createEnvelope, serializeEnvelope } from '@sentry/utils'; +import { makeNewXHRTransport, XHRTransportOptions } from '../../../src/transports/new-xhr'; + +const DEFAULT_XHR_TRANSPORT_OPTIONS: XHRTransportOptions = { + url: 'https://sentry.io/api/42/store/?sentry_key=123&sentry_version=7', +}; + +const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, [ + [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, +]); + +function createXHRMock() { + const retryAfterSeconds = 10; + + const xhrMock: Partial = { + open: jest.fn(), + send: jest.fn(), + setRequestHeader: jest.fn(), + readyState: 4, + status: 200, + response: 'Hello World!', + onreadystatechange: () => {}, + getResponseHeader: (header: string) => { + switch (header) { + case 'Retry-After': + return '10'; + case `${retryAfterSeconds}`: + return; + default: + return `${retryAfterSeconds}:error:scope`; + } + }, + }; + + //@ts-ignore + jest.spyOn(window, 'XMLHttpRequest').mockImplementation(() => xhrMock as XMLHttpRequest); + + return xhrMock; +} + +describe('NewXHRTransport', () => { + it('makes an XHR request to the given URL', done => { + const xhrMock: Partial = createXHRMock(); + + const transport = makeNewXHRTransport(DEFAULT_XHR_TRANSPORT_OPTIONS); + expect(xhrMock.open).toHaveBeenCalledTimes(0); + expect(xhrMock.setRequestHeader).toHaveBeenCalledTimes(0); + expect(xhrMock.send).toHaveBeenCalledTimes(0); + + transport.send(ERROR_ENVELOPE).then(res => { + expect(xhrMock.open).toHaveBeenCalledTimes(1); + expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); + expect(xhrMock.send).toBeCalledWith(serializeEnvelope(ERROR_ENVELOPE)); + + expect(res).toBeTruthy; + expect(res.status).toEqual('success'); + + done(); + }); + + //@ts-ignore + xhrMock.onreadystatechange(); + }); +}); From 98d9a3dfb7539ea5108dd09395191c0052d1b916 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 25 Mar 2022 15:56:37 +0100 Subject: [PATCH 3/7] rework xhr transport test --- packages/browser/src/transports/new-xhr.ts | 6 ++-- .../test/unit/transports/new-xhr.test.ts | 35 +++++++++++-------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/packages/browser/src/transports/new-xhr.ts b/packages/browser/src/transports/new-xhr.ts index 6a8c6c4ae625..7373b946a505 100644 --- a/packages/browser/src/transports/new-xhr.ts +++ b/packages/browser/src/transports/new-xhr.ts @@ -9,8 +9,8 @@ import { SyncPromise } from '@sentry/utils'; export interface XHRTransportOptions extends BaseTransportOptions { // TODO choose whatever is preferred here (I like record more for easier readability) - headers?: { [key: string]: string }; - // headers?: Record; + //headers?: { [key: string]: string }; + headers?: Record; } /** @@ -22,6 +22,7 @@ export function makeNewXHRTransport(options: XHRTransportOptions): NewTransport const xhr = new XMLHttpRequest(); xhr.onreadystatechange = (): void => { + // TODO make 4 a constant if (xhr.readyState === 4) { const response = { body: xhr.response, @@ -33,7 +34,6 @@ export function makeNewXHRTransport(options: XHRTransportOptions): NewTransport statusCode: xhr.status, }; - // TODO when to reject? is it necessary at all, given that createTransport handles rejections? resolve(response); } }; diff --git a/packages/browser/test/unit/transports/new-xhr.test.ts b/packages/browser/test/unit/transports/new-xhr.test.ts index a99e38c1ce4f..b56f7b430b44 100644 --- a/packages/browser/test/unit/transports/new-xhr.test.ts +++ b/packages/browser/test/unit/transports/new-xhr.test.ts @@ -33,33 +33,40 @@ function createXHRMock() { }, }; - //@ts-ignore + //@ts-ignore because TS thinks window doesn't have XMLHttpRequest jest.spyOn(window, 'XMLHttpRequest').mockImplementation(() => xhrMock as XMLHttpRequest); return xhrMock; } describe('NewXHRTransport', () => { - it('makes an XHR request to the given URL', done => { - const xhrMock: Partial = createXHRMock(); + const xhrMock: Partial = createXHRMock(); + + afterEach(() => { + jest.clearAllMocks(); + }); + afterAll(() => { + jest.restoreAllMocks(); + }); + + it('makes an XHR request to the given URL', done => { const transport = makeNewXHRTransport(DEFAULT_XHR_TRANSPORT_OPTIONS); expect(xhrMock.open).toHaveBeenCalledTimes(0); expect(xhrMock.setRequestHeader).toHaveBeenCalledTimes(0); expect(xhrMock.send).toHaveBeenCalledTimes(0); - transport.send(ERROR_ENVELOPE).then(res => { - expect(xhrMock.open).toHaveBeenCalledTimes(1); - expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); - expect(xhrMock.send).toBeCalledWith(serializeEnvelope(ERROR_ENVELOPE)); - - expect(res).toBeTruthy; - expect(res.status).toEqual('success'); + Promise.all([transport.send(ERROR_ENVELOPE), (xhrMock as XMLHttpRequest).onreadystatechange(null)]).then( + ([res]) => { + expect(xhrMock.open).toHaveBeenCalledTimes(1); + expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); + expect(xhrMock.send).toBeCalledWith(serializeEnvelope(ERROR_ENVELOPE)); - done(); - }); + expect(res).toBeDefined(); + expect(res.status).toEqual('success'); - //@ts-ignore - xhrMock.onreadystatechange(); + done(); + }, + ); }); }); From 4ef11f012e317979ad85167b2ca46d79a719f68c Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 28 Mar 2022 11:26:44 +0200 Subject: [PATCH 4/7] refactor transports test to use async/await instead of `done()` --- .../test/unit/transports/new-xhr.test.ts | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/browser/test/unit/transports/new-xhr.test.ts b/packages/browser/test/unit/transports/new-xhr.test.ts index b56f7b430b44..4deee14f3d46 100644 --- a/packages/browser/test/unit/transports/new-xhr.test.ts +++ b/packages/browser/test/unit/transports/new-xhr.test.ts @@ -33,8 +33,8 @@ function createXHRMock() { }, }; - //@ts-ignore because TS thinks window doesn't have XMLHttpRequest - jest.spyOn(window, 'XMLHttpRequest').mockImplementation(() => xhrMock as XMLHttpRequest); + // casting `window` as `any` because XMLHttpRequest is missing in Window (TS-only) + jest.spyOn(window as any, 'XMLHttpRequest').mockImplementation(() => xhrMock as XMLHttpRequest); return xhrMock; } @@ -50,23 +50,22 @@ describe('NewXHRTransport', () => { jest.restoreAllMocks(); }); - it('makes an XHR request to the given URL', done => { + it('makes an XHR request to the given URL', async () => { const transport = makeNewXHRTransport(DEFAULT_XHR_TRANSPORT_OPTIONS); expect(xhrMock.open).toHaveBeenCalledTimes(0); expect(xhrMock.setRequestHeader).toHaveBeenCalledTimes(0); expect(xhrMock.send).toHaveBeenCalledTimes(0); - Promise.all([transport.send(ERROR_ENVELOPE), (xhrMock as XMLHttpRequest).onreadystatechange(null)]).then( - ([res]) => { - expect(xhrMock.open).toHaveBeenCalledTimes(1); - expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); - expect(xhrMock.send).toBeCalledWith(serializeEnvelope(ERROR_ENVELOPE)); + const [res] = await Promise.all([ + transport.send(ERROR_ENVELOPE), + (xhrMock as XMLHttpRequest).onreadystatechange(null), + ]); - expect(res).toBeDefined(); - expect(res.status).toEqual('success'); + expect(xhrMock.open).toHaveBeenCalledTimes(1); + expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); + expect(xhrMock.send).toBeCalledWith(serializeEnvelope(ERROR_ENVELOPE)); - done(); - }, - ); + expect(res).toBeDefined(); + expect(res.status).toEqual('success'); }); }); From da67dd98034d20ad9778ede16d2f7a019a620195 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 28 Mar 2022 14:12:35 +0200 Subject: [PATCH 5/7] add tests for rate limit and custom headers --- packages/browser/src/transports/new-xhr.ts | 18 +++++-- .../test/unit/transports/new-xhr.test.ts | 49 ++++++++++++++++--- 2 files changed, 56 insertions(+), 11 deletions(-) diff --git a/packages/browser/src/transports/new-xhr.ts b/packages/browser/src/transports/new-xhr.ts index 7373b946a505..7f9cad6cdfa1 100644 --- a/packages/browser/src/transports/new-xhr.ts +++ b/packages/browser/src/transports/new-xhr.ts @@ -7,9 +7,17 @@ import { } from '@sentry/core'; import { SyncPromise } from '@sentry/utils'; +/** + * The DONE ready state for XmlHttpRequest + * + * Defining it here as a constant b/c XMLHttpRequest.DONE is not always defined + * (e.g. during testing, it is `undefined`) + * + * @see {@link https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/readyState} + */ +const XHR_READYSTATE_DONE = 4; + export interface XHRTransportOptions extends BaseTransportOptions { - // TODO choose whatever is preferred here (I like record more for easier readability) - //headers?: { [key: string]: string }; headers?: Record; } @@ -22,8 +30,7 @@ export function makeNewXHRTransport(options: XHRTransportOptions): NewTransport const xhr = new XMLHttpRequest(); xhr.onreadystatechange = (): void => { - // TODO make 4 a constant - if (xhr.readyState === 4) { + if (xhr.readyState === XHR_READYSTATE_DONE) { const response = { body: xhr.response, headers: { @@ -33,17 +40,18 @@ export function makeNewXHRTransport(options: XHRTransportOptions): NewTransport reason: xhr.statusText, statusCode: xhr.status, }; - resolve(response); } }; xhr.open('POST', options.url); + for (const header in options.headers) { if (Object.prototype.hasOwnProperty.call(options.headers, header)) { xhr.setRequestHeader(header, options.headers[header]); } } + xhr.send(request.body); }); } diff --git a/packages/browser/test/unit/transports/new-xhr.test.ts b/packages/browser/test/unit/transports/new-xhr.test.ts index 4deee14f3d46..a7c67e7b2396 100644 --- a/packages/browser/test/unit/transports/new-xhr.test.ts +++ b/packages/browser/test/unit/transports/new-xhr.test.ts @@ -21,7 +21,7 @@ function createXHRMock() { status: 200, response: 'Hello World!', onreadystatechange: () => {}, - getResponseHeader: (header: string) => { + getResponseHeader: jest.fn((header: string) => { switch (header) { case 'Retry-After': return '10'; @@ -30,7 +30,7 @@ function createXHRMock() { default: return `${retryAfterSeconds}:error:scope`; } - }, + }), }; // casting `window` as `any` because XMLHttpRequest is missing in Window (TS-only) @@ -56,16 +56,53 @@ describe('NewXHRTransport', () => { expect(xhrMock.setRequestHeader).toHaveBeenCalledTimes(0); expect(xhrMock.send).toHaveBeenCalledTimes(0); + await Promise.all([transport.send(ERROR_ENVELOPE), (xhrMock as XMLHttpRequest).onreadystatechange(null)]); + + expect(xhrMock.open).toHaveBeenCalledTimes(1); + expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); + expect(xhrMock.send).toHaveBeenCalledTimes(1); + expect(xhrMock.send).toHaveBeenCalledWith(serializeEnvelope(ERROR_ENVELOPE)); + }); + + it('returns the correct response', async () => { + const transport = makeNewXHRTransport(DEFAULT_XHR_TRANSPORT_OPTIONS); + const [res] = await Promise.all([ transport.send(ERROR_ENVELOPE), (xhrMock as XMLHttpRequest).onreadystatechange(null), ]); - expect(xhrMock.open).toHaveBeenCalledTimes(1); - expect(xhrMock.open).toHaveBeenCalledWith('POST', DEFAULT_XHR_TRANSPORT_OPTIONS.url); - expect(xhrMock.send).toBeCalledWith(serializeEnvelope(ERROR_ENVELOPE)); - expect(res).toBeDefined(); expect(res.status).toEqual('success'); }); + + it('sets rate limit response headers', async () => { + const transport = makeNewXHRTransport(DEFAULT_XHR_TRANSPORT_OPTIONS); + + await Promise.all([transport.send(ERROR_ENVELOPE), (xhrMock as XMLHttpRequest).onreadystatechange(null)]); + + expect(xhrMock.getResponseHeader).toHaveBeenCalledTimes(2); + expect(xhrMock.getResponseHeader).toHaveBeenCalledWith('X-Sentry-Rate-Limits'); + expect(xhrMock.getResponseHeader).toHaveBeenCalledWith('Retry-After'); + }); + + it('sets custom request headers', async () => { + const headers = { + referrerPolicy: 'strict-origin', + keepalive: 'true', + referrer: 'http://example.org', + }; + const options: XHRTransportOptions = { + ...DEFAULT_XHR_TRANSPORT_OPTIONS, + headers, + }; + + const transport = makeNewXHRTransport(options); + await Promise.all([transport.send(ERROR_ENVELOPE), (xhrMock as XMLHttpRequest).onreadystatechange(null)]); + + expect(xhrMock.setRequestHeader).toHaveBeenCalledTimes(3); + expect(xhrMock.setRequestHeader).toHaveBeenCalledWith('referrerPolicy', headers.referrerPolicy); + expect(xhrMock.setRequestHeader).toHaveBeenCalledWith('keepalive', headers.keepalive); + expect(xhrMock.setRequestHeader).toHaveBeenCalledWith('referrer', headers.referrer); + }); }); From aed77c1b8421dd8d8dbe0c496eee6b7a41d4dd7a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 28 Mar 2022 14:26:40 +0200 Subject: [PATCH 6/7] add newXhr creation in BrowserBackend --- packages/browser/src/backend.ts | 7 ++++++- packages/browser/src/transports/new-xhr.ts | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/browser/src/backend.ts b/packages/browser/src/backend.ts index d6dfe5d25ddc..4cdef3d11047 100644 --- a/packages/browser/src/backend.ts +++ b/packages/browser/src/backend.ts @@ -3,7 +3,7 @@ import { Event, EventHint, Options, Severity, Transport, TransportOptions } from import { supportsFetch } from '@sentry/utils'; import { eventFromException, eventFromMessage } from './eventbuilder'; -import { FetchTransport, makeNewFetchTransport, XHRTransport } from './transports'; +import { FetchTransport, makeNewFetchTransport, makeNewXHRTransport, XHRTransport } from './transports'; /** * Configuration options for the Sentry Browser SDK. @@ -77,6 +77,11 @@ export class BrowserBackend extends BaseBackend { this._newTransport = makeNewFetchTransport({ requestOptions, url }); return new FetchTransport(transportOptions); } + + this._newTransport = makeNewXHRTransport({ + url, + headers: transportOptions.headers, + }); return new XHRTransport(transportOptions); } } diff --git a/packages/browser/src/transports/new-xhr.ts b/packages/browser/src/transports/new-xhr.ts index 7f9cad6cdfa1..cd19b1de0cd4 100644 --- a/packages/browser/src/transports/new-xhr.ts +++ b/packages/browser/src/transports/new-xhr.ts @@ -18,7 +18,7 @@ import { SyncPromise } from '@sentry/utils'; const XHR_READYSTATE_DONE = 4; export interface XHRTransportOptions extends BaseTransportOptions { - headers?: Record; + headers?: { [key: string]: string }; } /** From 4672d150785a4e90c18e85b29fdd584ab426a95a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 29 Mar 2022 10:48:29 +0200 Subject: [PATCH 7/7] fix format error caught by linter --- packages/browser/test/unit/transports/new-xhr.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/browser/test/unit/transports/new-xhr.test.ts b/packages/browser/test/unit/transports/new-xhr.test.ts index a7c67e7b2396..5b3bbda313c7 100644 --- a/packages/browser/test/unit/transports/new-xhr.test.ts +++ b/packages/browser/test/unit/transports/new-xhr.test.ts @@ -1,5 +1,6 @@ import { EventEnvelope, EventItem } from '@sentry/types'; import { createEnvelope, serializeEnvelope } from '@sentry/utils'; + import { makeNewXHRTransport, XHRTransportOptions } from '../../../src/transports/new-xhr'; const DEFAULT_XHR_TRANSPORT_OPTIONS: XHRTransportOptions = {