From 8e0cab01c8a32f0b825259f7391651bce6ba191f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 30 Nov 2017 15:58:39 +0100 Subject: [PATCH 1/4] feat: Use Fetch instead of XHR when available --- src/raven.js | 54 ++++++++++--- src/utils.js | 5 ++ test/raven.test.js | 185 +++++++++++++++++++++++++++++++-------------- 3 files changed, 178 insertions(+), 66 deletions(-) diff --git a/src/raven.js b/src/raven.js index 1d2e9008fc14..f44953a69d6e 100644 --- a/src/raven.js +++ b/src/raven.js @@ -25,6 +25,7 @@ var isSameException = utils.isSameException; var isSameStacktrace = utils.isSameStacktrace; var parseUrl = utils.parseUrl; var fill = utils.fill; +var supportsFetch = utils.supportsFetch; var wrapConsoleMethod = require('./console').wrapMethod; @@ -1179,7 +1180,7 @@ Raven.prototype = { xhrproto, 'send', function(origSend) { - return function(data) { + return function() { // preserve arity var xhr = this; @@ -1227,12 +1228,12 @@ Raven.prototype = { ); } - if (autoBreadcrumbs.xhr && 'fetch' in _window) { + if (autoBreadcrumbs.xhr && supportsFetch()) { fill( _window, 'fetch', function(origFetch) { - return function(fn, t) { + return function() { // preserve arity // Make a copy of the arguments to prevent deoptimization // https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#32-leaking-arguments @@ -1256,6 +1257,11 @@ Raven.prototype = { url = '' + fetchInput; } + // if Sentry key appears in URL, don't capture, as it's our own request + if (url.indexOf(self._globalKey) !== -1) { + return origFetch.apply(this, args); + } + if (args[1] && args[1].method) { method = args[1].method; } @@ -1692,8 +1698,14 @@ Raven.prototype = { try { // If Retry-After is not in Access-Control-Expose-Headers, most // browsers will throw an exception trying to access it - retry = request.getResponseHeader('Retry-After'); - retry = parseInt(retry, 10) * 1000; // Retry-After is returned in seconds + if (supportsFetch()) { + retry = request.headers.get('Retry-After'); + } else { + retry = request.getResponseHeader('Retry-After'); + } + + // Retry-After is returned in seconds + retry = parseInt(retry, 10) * 1000; } catch (e) { /* eslint no-empty:0 */ } @@ -1882,6 +1894,32 @@ Raven.prototype = { }, _makeRequest: function(opts) { + // Auth is intentionally sent as part of query string (NOT as custom HTTP header) to avoid preflight CORS requests + var url = opts.url + '?' + urlencode(opts.auth); + + if (supportsFetch()) { + return _window + .fetch(url, { + method: 'POST', + body: stringify(opts.data) + }) + .then(function(response) { + if (response.ok) { + opts.onSuccess && opts.onSuccess(); + } else { + var error = new Error('Sentry error code: ' + response.status); + // It's called request only to keep compatibility with XHR interface + // and not add more redundant checks in setBackoffState method + error.request = response; + opts.onError && opts.onError(error); + } + }) + ['catch'](function() { + opts.onError && + opts.onError(new Error('Sentry error code: network unavailable')); + }); + } + var request = _window.XMLHttpRequest && new _window.XMLHttpRequest(); if (!request) return; @@ -1890,8 +1928,6 @@ Raven.prototype = { if (!hasCORS) return; - var url = opts.url; - if ('withCredentials' in request) { request.onreadystatechange = function() { if (request.readyState !== 4) { @@ -1923,9 +1959,7 @@ Raven.prototype = { } } - // NOTE: auth is intentionally sent as part of query string (NOT as custom - // HTTP header) so as to avoid preflight CORS requests - request.open('POST', url + '?' + urlencode(opts.auth)); + request.open('POST', url); request.send(stringify(opts.data)); }, diff --git a/src/utils.js b/src/utils.js index 90efbfcf449d..031430a92499 100644 --- a/src/utils.js +++ b/src/utils.js @@ -56,6 +56,10 @@ function supportsErrorEvent() { } } +function supportsFetch() { + return 'fetch' in _window; +} + function wrappedCallback(callback) { function dataCallback(data, original) { var normalizedData = callback(data) || data; @@ -373,6 +377,7 @@ module.exports = { isString: isString, isEmptyObject: isEmptyObject, supportsErrorEvent: supportsErrorEvent, + supportsFetch: supportsFetch, wrappedCallback: wrappedCallback, each: each, objectMerge: objectMerge, diff --git a/test/raven.test.js b/test/raven.test.js index 39d3b53baa6b..c9b9efbd6b78 100644 --- a/test/raven.test.js +++ b/test/raven.test.js @@ -23,6 +23,7 @@ _Raven.prototype._getUuid = function() { var utils = require('../src/utils'); var joinRegExp = utils.joinRegExp; var supportsErrorEvent = utils.supportsErrorEvent; +var supportsFetch = utils.supportsFetch; // window.console must be stubbed in for browsers that don't have it if (typeof window.console === 'undefined') { @@ -1503,12 +1504,22 @@ describe('globals', function() { var opts = Raven._makeRequest.lastCall.args[0]; var mockError = new Error('401: Unauthorized'); mockError.request = { - status: 401, - getResponseHeader: sinon - .stub() - .withArgs('Retry-After') - .returns('2') + status: 401 }; + + var retryAfterStub = sinon + .stub() + .withArgs('Retry-After') + .returns('2'); + + if (supportsFetch) { + mockError.request.headers = { + get: retryAfterStub + }; + } else { + mockError.request.getResponseHeader = retryAfterStub; + } + opts.onError(mockError); assert.equal(Raven._backoffStart, 100); // clock is at 100ms @@ -1653,74 +1664,136 @@ describe('globals', function() { }); describe('makeRequest', function() { - beforeEach(function() { - // NOTE: can't seem to call useFakeXMLHttpRequest via sandbox; must - // restore manually - this.xhr = sinon.useFakeXMLHttpRequest(); - var requests = (this.requests = []); + describe('using Fetch API', function() { + afterEach(function() { + window.fetch.restore(); + }); - this.xhr.onCreate = function(xhr) { - requests.push(xhr); - }; - }); + it('should create an XMLHttpRequest object with body as JSON payload', function() { + this.sinon.spy(window, 'fetch'); + + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions + }); + + assert.deepEqual(window.fetch.lastCall.args, [ + 'http://localhost/?a=1&b=2', + { + method: 'POST', + body: '{"foo":"bar"}' + } + ]); + }); - afterEach(function() { - this.xhr.restore(); + it('should pass a request object to onError', function(done) { + sinon.stub(window, 'fetch'); + window.fetch.returns( + Promise.resolve( + new window.Response('{"foo":"bar"}', { + ok: false, + status: 429, + headers: { + 'Content-type': 'text/html' + } + }) + ) + ); + + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions, + onError: function(error) { + assert.equal(error.request.status, 429); + done(); + } + }); + }); }); - it('should create an XMLHttpRequest object with body as JSON payload', function() { - XMLHttpRequest.prototype.withCredentials = true; + describe('using XHR API', function() { + var origFetch = window.fetch; + var xhr; + var requests; - Raven._makeRequest({ - url: 'http://localhost/', - auth: {a: '1', b: '2'}, - data: {foo: 'bar'}, - options: Raven._globalOptions + before(function() { + delete window.fetch; }); - var lastXhr = this.requests[this.requests.length - 1]; - assert.equal(lastXhr.requestBody, '{"foo":"bar"}'); - assert.equal(lastXhr.url, 'http://localhost/?a=1&b=2'); - }); + after(function() { + window.fetch = origFetch; + }); + + beforeEach(function() { + // NOTE: can't seem to call useFakeXMLHttpRequest via sandbox; must restore manually + xhr = sinon.useFakeXMLHttpRequest(); + requests = []; - it('should no-op if CORS is not supported', function() { - delete XMLHttpRequest.prototype.withCredentials; - var oldSupportsCORS = sinon.xhr.supportsCORS; - sinon.xhr.supportsCORS = false; + XMLHttpRequest.prototype.withCredentials = true; - var oldXDR = window.XDomainRequest; - window.XDomainRequest = undefined; + xhr.onCreate = function(xhr) { + requests.push(xhr); + }; + }); - Raven._makeRequest({ - url: 'http://localhost/', - auth: {a: '1', b: '2'}, - data: {foo: 'bar'}, - options: Raven._globalOptions + afterEach(function() { + xhr.restore(); }); - assert.equal(this.requests.length, 1); // the "test" xhr - assert.equal(this.requests[0].readyState, 0); + it('should create an XMLHttpRequest object with body as JSON payload', function() { + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions + }); - sinon.xhr.supportsCORS = oldSupportsCORS; - window.XDomainRequest = oldXDR; - }); + var lastXhr = requests[requests.length - 1]; + assert.equal(lastXhr.requestBody, '{"foo":"bar"}'); + assert.equal(lastXhr.url, 'http://localhost/?a=1&b=2'); + }); - it('should pass a request object to onError', function(done) { - XMLHttpRequest.prototype.withCredentials = true; + it('should pass a request object to onError', function(done) { + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions, + onError: function(error) { + assert.equal(error.request.status, 429); + done(); + } + }); - Raven._makeRequest({ - url: 'http://localhost/', - auth: {a: '1', b: '2'}, - data: {foo: 'bar'}, - options: Raven._globalOptions, - onError: function(error) { - assert.equal(error.request.status, 429); - done(); - } + var lastXhr = requests[requests.length - 1]; + lastXhr.respond(429, {'Content-Type': 'text/html'}, 'Too many requests'); }); - var lastXhr = this.requests[this.requests.length - 1]; - lastXhr.respond(429, {'Content-Type': 'text/html'}, 'Too many requests'); + it('should no-op if CORS is not supported', function() { + delete XMLHttpRequest.prototype.withCredentials; + var oldSupportsCORS = sinon.xhr.supportsCORS; + sinon.xhr.supportsCORS = false; + + var oldXDR = window.XDomainRequest; + window.XDomainRequest = undefined; + + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions + }); + + assert.equal(requests.length, 1); // the "test" xhr + assert.equal(requests[0].readyState, 0); + + sinon.xhr.supportsCORS = oldSupportsCORS; + window.XDomainRequest = oldXDR; + }); }); }); From 49c8fa3ee0198f042a5d1f2709bbbc1aecd43513 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 30 Nov 2017 16:17:11 +0100 Subject: [PATCH 2/4] Just travis things --- .travis.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 6ace5aa61e3c..c3a5fce90f6a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,7 +1,7 @@ branches: only: - master -sudo: false +sudo: required language: node_js node_js: - '8' @@ -13,5 +13,8 @@ addons: chrome: stable firefox: latest sauce_connect: true +before_script: + - "sudo chown root /opt/google/chrome/chrome-sandbox" + - "sudo chmod 4755 /opt/google/chrome/chrome-sandbox" script: - npm run test && if [ "$TRAVIS_SECURE_ENV_VARS" == "true" ]; then npm run test:ci; else exit 0; fi From c32ad411117f5b9b4952af0b744a116db3a8f8cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 30 Nov 2017 16:42:36 +0100 Subject: [PATCH 3/4] Lower concurrency on karma browsers to 2 --- karma.config.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/karma.config.js b/karma.config.js index d5d1f7051917..9de77be69cca 100644 --- a/karma.config.js +++ b/karma.config.js @@ -60,7 +60,7 @@ module.exports = { // Concurrency level // how many browser should be started simultaneous - concurrency: Infinity, + concurrency: 2, client: { mocha: { From 62f8413f3992723a9412d9409546a4eca2d49a00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Thu, 30 Nov 2017 17:29:39 +0100 Subject: [PATCH 4/4] Run fetch tests for supported browsers only --- test/raven.test.js | 141 ++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 59 deletions(-) diff --git a/test/raven.test.js b/test/raven.test.js index c9b9efbd6b78..53333f9f9f0e 100644 --- a/test/raven.test.js +++ b/test/raven.test.js @@ -1496,7 +1496,10 @@ describe('globals', function() { assert.equal(Raven._backoffDuration, 2000); }); - it('should set backoffDuration to value of Retry-If header if present', function() { + it('should set backoffDuration to value of Retry-If header if present - XHR API', function() { + var origFetch = window.fetch; + delete window.fetch; + this.sinon.stub(Raven, 'isSetup').returns(true); this.sinon.stub(Raven, '_makeRequest'); @@ -1504,28 +1507,46 @@ describe('globals', function() { var opts = Raven._makeRequest.lastCall.args[0]; var mockError = new Error('401: Unauthorized'); mockError.request = { - status: 401 + status: 401, + getResponseHeader: sinon + .stub() + .withArgs('Retry-After') + .returns('2') }; - var retryAfterStub = sinon - .stub() - .withArgs('Retry-After') - .returns('2'); - - if (supportsFetch) { - mockError.request.headers = { - get: retryAfterStub - }; - } else { - mockError.request.getResponseHeader = retryAfterStub; - } - opts.onError(mockError); assert.equal(Raven._backoffStart, 100); // clock is at 100ms assert.equal(Raven._backoffDuration, 2000); // converted to ms, int + + window.fetch = origFetch; }); + if (supportsFetch()) { + it('should set backoffDuration to value of Retry-If header if present - FETCH API', function() { + this.sinon.stub(Raven, 'isSetup').returns(true); + this.sinon.stub(Raven, '_makeRequest'); + + Raven._send({message: 'bar'}); + var opts = Raven._makeRequest.lastCall.args[0]; + var mockError = new Error('401: Unauthorized'); + mockError.request = { + status: 401, + headers: { + get: sinon + .stub() + .withArgs('Retry-After') + .returns('2') + } + }; + + opts.onError(mockError); + + assert.equal(Raven._backoffStart, 100); // clock is at 100ms + assert.equal(Raven._backoffDuration, 2000); // converted to ms, int + }); + } + it('should reset backoffDuration and backoffStart if onSuccess is fired (200)', function() { this.sinon.stub(Raven, 'isSetup').returns(true); this.sinon.stub(Raven, '_makeRequest'); @@ -1664,56 +1685,58 @@ describe('globals', function() { }); describe('makeRequest', function() { - describe('using Fetch API', function() { - afterEach(function() { - window.fetch.restore(); - }); - - it('should create an XMLHttpRequest object with body as JSON payload', function() { - this.sinon.spy(window, 'fetch'); - - Raven._makeRequest({ - url: 'http://localhost/', - auth: {a: '1', b: '2'}, - data: {foo: 'bar'}, - options: Raven._globalOptions + if (supportsFetch()) { + describe('using Fetch API', function() { + afterEach(function() { + window.fetch.restore(); }); - assert.deepEqual(window.fetch.lastCall.args, [ - 'http://localhost/?a=1&b=2', - { - method: 'POST', - body: '{"foo":"bar"}' - } - ]); - }); + it('should create an XMLHttpRequest object with body as JSON payload', function() { + this.sinon.spy(window, 'fetch'); - it('should pass a request object to onError', function(done) { - sinon.stub(window, 'fetch'); - window.fetch.returns( - Promise.resolve( - new window.Response('{"foo":"bar"}', { - ok: false, - status: 429, - headers: { - 'Content-type': 'text/html' - } - }) - ) - ); + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions + }); - Raven._makeRequest({ - url: 'http://localhost/', - auth: {a: '1', b: '2'}, - data: {foo: 'bar'}, - options: Raven._globalOptions, - onError: function(error) { - assert.equal(error.request.status, 429); - done(); - } + assert.deepEqual(window.fetch.lastCall.args, [ + 'http://localhost/?a=1&b=2', + { + method: 'POST', + body: '{"foo":"bar"}' + } + ]); + }); + + it('should pass a request object to onError', function(done) { + sinon.stub(window, 'fetch'); + window.fetch.returns( + Promise.resolve( + new window.Response('{"foo":"bar"}', { + ok: false, + status: 429, + headers: { + 'Content-type': 'text/html' + } + }) + ) + ); + + Raven._makeRequest({ + url: 'http://localhost/', + auth: {a: '1', b: '2'}, + data: {foo: 'bar'}, + options: Raven._globalOptions, + onError: function(error) { + assert.equal(error.request.status, 429); + done(); + } + }); }); }); - }); + } describe('using XHR API', function() { var origFetch = window.fetch;