From 7aae0a31d88af22240f4479d3dbf275566c5400c Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Thu, 7 Jan 2016 11:03:10 -0800 Subject: [PATCH] fix(ajax): ensure post sending values - refactors ajax to send posted data properly - adds logic for serializing data to url-formdata - creates custom XMLHttpRequest mock to enable better testing and integrate better with the library - ensures proper content type setting - corrects tests that were false positives because of jasmine-ajax --- spec/helpers/ajax-helper.js | 126 +++++++++++++++++++++++++++--- spec/observables/dom/ajax-spec.js | 118 +++++++++++++++++----------- src/observable/dom/ajax.ts | 81 +++++++++++++------ 3 files changed, 249 insertions(+), 76 deletions(-) diff --git a/spec/helpers/ajax-helper.js b/spec/helpers/ajax-helper.js index b031769694..20b293abd1 100644 --- a/spec/helpers/ajax-helper.js +++ b/spec/helpers/ajax-helper.js @@ -1,14 +1,120 @@ -var jasmineCore = require('jasmine-core'); +var root = require('../../dist/cjs/util/root').root; -// jasmine-ajax need this -global.getJasmineRequireObj = function () { - return jasmineCore; +var requests = []; +var recentRequest = null; + +function MockXMLHttpRequest() { + this.previousRequest = recentRequest; + recentRequest = this; + requests.push(this); + this.requestHeaders = {}; + this.responseType = ''; + this.eventHandlers = []; + this.readyState = 0; +} + +MockXMLHttpRequest.prototype = { + send: function (data) { + this.data = data; + }, + + open: function (method, url, async, user, password) { + this.method = method; + this.url = url; + this.async = async; + this.user = user; + this.password = password; + this.readyState = 1; + this.triggerEvent('readystatechange'); + }, + + setRequestHeader: function (key, value) { + this.requestHeaders[key] = value; + }, + + addEventListener: function (name, handler) { + this.eventHandlers.push({ name: name, handler: handler }); + }, + + removeEventListener: function (name, handler) { + for (var i = this.eventHandlers.length - 1; i--;) { + var eh = this.eventHandlers[i]; + if (eh.name === name && eh.handler === handler) { + this.eventHandlers.splice(i, 1); + } + } + }, + + throwError: function (err) { + // TODO: something better with errors + this.triggerEvent('error'); + }, + + respondWith: function (response) { + this.readyState = 4; + this.responseHeaders = { + 'Content-Type': response.contentType || 'text/plain' + }; + this.status = response.status || 200; + this.responseText = response.responseText; + if (!('response' in response)) { + switch (this.responseType) { + case 'json': + try { + this.response = JSON.parse(response.responseText); + } catch (err) { + throw new Error('unable to JSON.parse: \n' + response.responseText); + } + break; + case 'text': + this.response = response.responseText; + break; + default: + throw new Error('unhandled type "' + this.responseType + '"'); + } + } + // TODO: pass better event to onload. + this.triggerEvent('load'); + this.triggerEvent('readystatechange'); + }, + + triggerEvent: function (name, eventObj) { + // TODO: create a better default event + e = eventObj || {}; + + if (this['on' + name]) { + this['on' + name](e); + } + + this.eventHandlers.forEach(function (eh) { + if (eh.name === name) { + eh.handler.call(this, e); + } + }); + } +}; + +MockXMLHttpRequest.mostRecent = function () { + return recentRequest; }; -// XMLHttpRequest in node -global.XMLHttpRequest = require('xmlhttprequest').XMLHttpRequest; +MockXMLHttpRequest.allRequests = function () { + return requests; +}; + +var gXHR; +var rXHR; + +global.setupMockXHR = function () { + gXHR = global.XMLHttpRequest; + rXHR = root.XMLHttpRequest; + global.XMLHttpRequest = MockXMLHttpRequest; + root.XMLHttpRequest = MockXMLHttpRequest; +}; -var w = global.window; -global.window = global; -require.call(global, 'jasmine-ajax'); -global.window = w; +global.teardownMockXHR = function () { + global.XMLHttpRequest = gXHR; + root.XMLHttpRequest = rXHR; + requests.length = 0; + recentRequest = null; +}; \ No newline at end of file diff --git a/spec/observables/dom/ajax-spec.js b/spec/observables/dom/ajax-spec.js index 4373906df7..5cf9155c84 100644 --- a/spec/observables/dom/ajax-spec.js +++ b/spec/observables/dom/ajax-spec.js @@ -8,11 +8,11 @@ function noop() { describe('Observable.ajax', function () { beforeEach(function () { - jasmine.Ajax.install(); + setupMockXHR(); }); afterEach(function () { - jasmine.Ajax.uninstall(); + teardownMockXHR(); }); it('should set headers', function () { @@ -26,7 +26,7 @@ describe('Observable.ajax', function () { }) .subscribe(); - var request = jasmine.Ajax.requests.mostRecent(); + var request = XMLHttpRequest.mostRecent(); expect(request.url).toBe('/talk-to-me-goose'); expect(request.requestHeaders).toEqual({ @@ -52,15 +52,13 @@ describe('Observable.ajax', function () { }) .subscribe(function(x) { result = x; - }, function () { - throw 'should not have been called'; - }, function () { + }, null, function () { complete = true; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + XMLHttpRequest.mostRecent().respondWith({ 'status': 200, 'contentType': 'application/json', 'responseText': expected @@ -90,9 +88,9 @@ describe('Observable.ajax', function () { throw 'should not complete'; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + XMLHttpRequest.mostRecent().respondWith({ 'status': 200, 'contentType': 'application/json', 'responseText': expected @@ -135,15 +133,13 @@ describe('Observable.ajax', function () { }) .subscribe(function(x) { result = x; - }, function () { - throw 'should not have been called'; - }, function () { + }, null, function () { complete = true; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + XMLHttpRequest.mostRecent().respondWith({ 'status': 200, 'contentType': 'application/json', 'responseText': JSON.stringify(expected) @@ -162,10 +158,10 @@ describe('Observable.ajax', function () { url: '/flibbertyJibbet', normalizeError: function (e, xhr, type) { return xhr.response || xhr.responseText; - } + }, + responseType: 'text' }) .subscribe(function (x) { - console.log(x); throw 'should not next'; }, function (x) { error = x; @@ -173,9 +169,9 @@ describe('Observable.ajax', function () { throw 'should not complete'; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + XMLHttpRequest.mostRecent().respondWith({ 'status': 404, 'contentType': 'text/plain', 'responseText': 'Wee! I am text!' @@ -195,10 +191,10 @@ describe('Observable.ajax', function () { url: '/flibbertyJibbet', normalizeError: function (e, xhr, type) { return xhr.response || xhr.responseText; - } + }, + responseType: 'text' }) .subscribe(function (x) { - console.log(x); throw 'should not next'; }, function (x) { error = x; @@ -206,9 +202,9 @@ describe('Observable.ajax', function () { throw 'should not complete'; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + XMLHttpRequest.mostRecent().respondWith({ 'status': 300, 'contentType': 'text/plain', 'responseText': 'Wee! I am text!' @@ -232,8 +228,8 @@ describe('Observable.ajax', function () { throw 'should not have been called'; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); + XMLHttpRequest.mostRecent().respondWith({ 'status': 200, 'contentType': 'text/plain', 'responseText': expected @@ -255,8 +251,8 @@ describe('Observable.ajax', function () { throw 'should not have been called'; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); + XMLHttpRequest.mostRecent().respondWith({ 'status': 500, 'contentType': 'text/plain', 'responseText': expected @@ -265,7 +261,7 @@ describe('Observable.ajax', function () { describe('ajax.get', function () { it('should succeed on 200', function () { - var expected = 'some response'; + var expected = { foo: 'bar' }; var result; var complete = false; @@ -273,55 +269,89 @@ describe('Observable.ajax', function () { .ajax.get('/flibbertyJibbet') .subscribe(function(x) { result = x; - }, function () { - throw 'should not have been called'; - }, function () { + }, null, function () { complete = true; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + var request = XMLHttpRequest.mostRecent(); - jasmine.Ajax.requests.mostRecent().respondWith({ + expect(request.url).toBe('/flibbertyJibbet'); + + request.respondWith({ 'status': 200, 'contentType': 'application/json', - 'responseText': expected + 'responseText': JSON.stringify(expected) }); - expect(result).toBe(expected); + expect(result).toEqual(expected); expect(complete).toBe(true); }); it('should succeed on 200 with a resultSelector', function () { - var expected = 'hahahahaha'; + var expected = { larf: 'hahahahaha' }; var result, innerResult; var complete = false; Rx.Observable .ajax.get('/flibbertyJibbet', function (x) { innerResult = x; - return x.response.toUpperCase(); + return x.response.larf.toUpperCase(); }) .subscribe(function(x) { result = x; - }, function () { - throw 'should not have been called'; - }, function () { + }, null , function () { complete = true; }); - expect(jasmine.Ajax.requests.mostRecent().url).toBe('/flibbertyJibbet'); + expect(XMLHttpRequest.mostRecent().url).toBe('/flibbertyJibbet'); - jasmine.Ajax.requests.mostRecent().respondWith({ + XMLHttpRequest.mostRecent().respondWith({ 'status': 200, 'contentType': 'application/json', - 'responseText': expected + 'responseText': JSON.stringify(expected) }); expect(innerResult.xhr).toBeDefined(); - expect(innerResult.response).toBe(expected); + expect(innerResult.response).toEqual({ larf: 'hahahahaha' }); expect(result).toBe('HAHAHAHAHA'); expect(complete).toBe(true); }); }); + + describe('ajax.post', function () { + it('should succeed on 200', function () { + var expected = { foo: 'bar', hi: 'there you' }; + var result; + var complete = false; + + Rx.Observable + .ajax.post('/flibbertyJibbet', expected) + .subscribe(function(x) { + result = x; + }, null , function () { + complete = true; + }); + + var request = XMLHttpRequest.mostRecent(); + + expect(request.method).toBe('POST'); + expect(request.url).toBe('/flibbertyJibbet'); + expect(request.requestHeaders).toEqual({ + 'X-Requested-With': 'XMLHttpRequest', + 'Content-Type': 'application/x-www-form-urlencoded; charset=UTF-8' + }) + + request.respondWith({ + 'status': 200, + 'contentType': 'application/json', + 'responseText': JSON.stringify(expected) + }); + + expect(request.data).toEqual('foo=bar&hi=there%20you'); + expect(result.response).toEqual(expected); + expect(complete).toBe(true); + }); + }); }); + diff --git a/src/observable/dom/ajax.ts b/src/observable/dom/ajax.ts index 0593564cb1..3702fa955e 100644 --- a/src/observable/dom/ajax.ts +++ b/src/observable/dom/ajax.ts @@ -113,7 +113,7 @@ export class AjaxObservable extends Observable { private request: AjaxRequest; - constructor(options: string | AjaxRequest) { + constructor(urlOrRequest: string | AjaxRequest) { super(); const request: AjaxRequest = { @@ -126,22 +126,15 @@ export class AjaxObservable extends Observable { timeout: 0 }; - if (typeof options === 'string') { - request.url = options; + if (typeof urlOrRequest === 'string') { + request.url = urlOrRequest; } else { - for (const prop in options) { - if (options.hasOwnProperty(prop)) { - request[prop] = options[prop]; + for (const prop in urlOrRequest) { + if (urlOrRequest.hasOwnProperty(prop)) { + request[prop] = urlOrRequest[prop]; } } } - request.headers = request.headers || {}; - - if (!request.crossDomain && !request.headers['X-Requested-With']) { - request.headers['X-Requested-With'] = 'XMLHttpRequest'; - } - - request.hasContent = request.body !== undefined; this.request = request; } @@ -158,6 +151,22 @@ export class AjaxSubscriber extends Subscriber { constructor(destination: Subscriber, public request: AjaxRequest) { super(destination); + + const headers = request.headers = request.headers || {}; + + // force CORS if requested + if (!request.crossDomain && !headers['X-Requested-With']) { + headers['X-Requested-With'] = 'XMLHttpRequest'; + } + + // ensure content type is set + if (!('Content-Type' in headers)) { + headers['Content-Type'] = 'application/x-www-form-urlencoded; charset=UTF-8'; + } + + // properly serialize body + request.body = this.serializeBody(request.body, request.headers['Content-Type']); + this.resultSelector = request.resultSelector; this.send(); } @@ -182,12 +191,12 @@ export class AjaxSubscriber extends Subscriber { private send(): XMLHttpRequest { const { request, - request: { user, method, url, async, password, headers } + request: { user, method, url, async, password, headers, body } } = this; const createXHR = request.createXHR; - const xhr = tryCatch(createXHR).call(request); + const xhr: XMLHttpRequest = tryCatch(createXHR).call(request); - if (xhr === errorObject) { + if (xhr === errorObject) { this.error(errorObject.e); } else { this.xhr = xhr; @@ -210,17 +219,39 @@ export class AjaxSubscriber extends Subscriber { xhr.responseType = request.responseType; // set headers - this.setupHeaders(xhr, headers); + this.setHeaders(xhr, headers); // now set up the events this.setupEvents(xhr, request); // finally send the request - xhr.send(); + if (body) { + xhr.send(body); + } else { + xhr.send(); + } } } - private setupHeaders(xhr: XMLHttpRequest, headers: Object) { + private serializeBody(body: any, contentType: string) { + if (!body || typeof body === 'string') { + return body; + } + + const splitIndex = contentType.indexOf(';'); + if (splitIndex !== -1) { + contentType = contentType.substring(0, splitIndex); + } + + switch (contentType) { + case 'application/x-www-form-urlencoded': + return Object.keys(body).map(key => `${key}=${encodeURI(body[key])}`).join('&'); + case 'application/json': + return JSON.stringify(body); + } + } + + private setHeaders(xhr: XMLHttpRequest, headers: Object) { for (let key in headers) { if (headers.hasOwnProperty(key)) { xhr.setRequestHeader(key, headers[key]); @@ -324,9 +355,15 @@ export class AjaxResponse { constructor(public originalEvent: Event, public xhr: XMLHttpRequest, public request: AjaxRequest) { this.status = xhr.status; const responseType = xhr.responseType; - let response = ('response' in xhr) ? xhr.response : xhr.responseText; - if (responseType === 'json') { - response = JSON.parse(response || ''); + let response; + if ('response' in xhr) { + response = xhr.response; + } else { + if (request.responseType === 'json') { + response = JSON.parse(xhr.responseText); + } else { + response = xhr.responseText; + } } this.responseText = xhr.responseText; this.responseType = responseType;