From 6ba374e54a257441ea31d1973eb098cfc885f266 Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Wed, 31 Aug 2016 10:33:16 -0700 Subject: [PATCH] fix(AjaxObservable): return null value from JSON.Parse (#1904) * Return null value from JSON.Parse JSON.parse('') will throw a syntax error (in any browser). Fix is to return a null value. I saw this when my server returned HTTP 204 No Content. * added ajax tests for 204 No Content * mock the behavior of IE unit test for IE * pascal case class name. added missing semicolon. * Use 'null' instead of '' for JSON.parse * remove empty constructor * fix(AjaxObservable): return null value from JSON.Parse JSON.parse('') will throw a syntax error (in any browser). Fix is to return a null value. This happens during an HTTP 204 'No Content' in IE. Added a mock for InternetExplorer. Added unit tests for HTTP 204. closes #1381 --- spec/helpers/ajax-helper.ts | 56 ++++++++++++---- spec/observables/dom/ajax-spec.ts | 98 +++++++++++++++++++++++++++- src/observable/dom/AjaxObservable.ts | 4 +- 3 files changed, 144 insertions(+), 14 deletions(-) diff --git a/spec/helpers/ajax-helper.ts b/spec/helpers/ajax-helper.ts index 244a6e345d..9c112c8635 100644 --- a/spec/helpers/ajax-helper.ts +++ b/spec/helpers/ajax-helper.ts @@ -117,7 +117,7 @@ export class MockXMLHttpRequest { private previousRequest: MockXMLHttpRequest; - private responseType: string = ''; + protected responseType: string = ''; private eventHandlers: Array = []; private readyState: number = 0; @@ -125,9 +125,9 @@ export class MockXMLHttpRequest { private password: any; private responseHeaders: any; - private status: any; - private responseText: string; - private response: any; + protected status: any; + protected responseText: string; + protected response: any; url: any; method: any; @@ -176,6 +176,18 @@ export class MockXMLHttpRequest { this.triggerEvent('error'); } + protected jsonResponseValue(response: any) { + try { + this.response = JSON.parse(response.responseText); + } catch (err) { + throw new Error('unable to JSON.parse: \n' + response.responseText); + } + } + + protected defaultResponseValue() { + throw new Error('unhandled type "' + this.responseType + '"'); + } + respondWith(response: any): void { this.readyState = 4; this.responseHeaders = { @@ -186,17 +198,13 @@ export class MockXMLHttpRequest { 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); - } + this.jsonResponseValue(response); break; case 'text': this.response = response.responseText; break; default: - throw new Error('unhandled type "' + this.responseType + '"'); + this.defaultResponseValue(); } } // TODO: pass better event to onload. @@ -218,4 +226,30 @@ export class MockXMLHttpRequest { } }); } -} \ No newline at end of file +} + +export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest { + + private mockHttp204() { + this.responseType = ''; + this.responseText = ''; + this.response = ''; + } + + protected jsonResponseValue(response: any) { + if (this.status == 204) { + this.mockHttp204(); + return; + } + return super.jsonResponseValue(response); + } + + protected defaultResponseValue() { + if (this.status == 204) { + this.mockHttp204(); + return; + } + return super.defaultResponseValue(); + } + +} diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 444d146e36..6b36531984 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -2,7 +2,7 @@ import {expect} from 'chai'; import * as sinon from 'sinon'; import * as Rx from '../../../dist/cjs/Rx'; import {root} from '../../../dist/cjs/util/root'; -import {MockXMLHttpRequest} from '../../helpers/ajax-helper'; +import {MockXMLHttpRequest, MockXMLHttpRequestInternetExplorer} from '../../helpers/ajax-helper'; declare const global: any; @@ -440,6 +440,33 @@ describe('Observable.ajax', () => { expect(complete).to.be.true; }); + it('should succeed on 204 No Content', () => { + const expected = null; + let result; + let complete = false; + + Rx.Observable + .ajax.get('/flibbertyJibbet') + .subscribe(x => { + result = x.response; + }, null, () => { + complete = true; + }); + + const request = MockXMLHttpRequest.mostRecent; + + expect(request.url).to.equal('/flibbertyJibbet'); + + request.respondWith({ + 'status': 204, + 'contentType': 'application/json', + 'responseText': expected + }); + + expect(result).to.deep.equal(expected); + expect(complete).to.be.true; + }); + it('should able to select json response via getJSON', () => { const expected = { foo: 'bar' }; let result; @@ -469,6 +496,7 @@ describe('Observable.ajax', () => { }); describe('ajax.post', () => { + it('should succeed on 200', () => { const expected = { foo: 'bar', hi: 'there you' }; let result: Rx.AjaxResponse; @@ -501,5 +529,73 @@ describe('Observable.ajax', () => { expect(result.response).to.deep.equal(expected); expect(complete).to.be.true; }); + + it('should succeed on 204 No Content', () => { + const expected = null; + let result: Rx.AjaxResponse; + let complete = false; + + Rx.Observable + .ajax.post('/flibbertyJibbet', expected) + .subscribe(x => { + result = x; + }, null, () => { + complete = true; + }); + + const request = MockXMLHttpRequest.mostRecent; + + expect(request.method).to.equal('POST'); + expect(request.url).to.equal('/flibbertyJibbet'); + expect(request.requestHeaders).to.deep.equal({ + 'X-Requested-With': 'XMLHttpRequest', + 'Content-Type': 'application/x-www-form-urlencoded; charset=UTF-8' + }); + + request.respondWith({ + 'status': 204, + 'contentType': 'application/json', + 'responseType': 'json', + 'responseText': expected + }); + + expect(result.response).to.equal(expected); + expect(complete).to.be.true; + }); + + it('should succeed in IE on 204 No Content', () => { + const expected = null; + let result: Rx.AjaxResponse; + let complete = false; + + root.XMLHttpRequest = MockXMLHttpRequestInternetExplorer; + + Rx.Observable + .ajax.post('/flibbertyJibbet', expected) + .subscribe(x => { + result = x; + }, null, () => { + complete = true; + }); + + const request = MockXMLHttpRequest.mostRecent; + + expect(request.method).to.equal('POST'); + expect(request.url).to.equal('/flibbertyJibbet'); + expect(request.requestHeaders).to.deep.equal({ + 'X-Requested-With': 'XMLHttpRequest', + 'Content-Type': 'application/x-www-form-urlencoded; charset=UTF-8' + }); + + //IE behavior: IE does not provide the a responseText property, so also exercise the code which handles this. + request.respondWith({ + 'status': 204, + 'contentType': 'application/json' + }); + + expect(result.response).to.equal(expected); + expect(complete).to.be.true; + }); + }); }); \ No newline at end of file diff --git a/src/observable/dom/AjaxObservable.ts b/src/observable/dom/AjaxObservable.ts index 0801c27914..9ae9916899 100644 --- a/src/observable/dom/AjaxObservable.ts +++ b/src/observable/dom/AjaxObservable.ts @@ -396,9 +396,9 @@ export class AjaxResponse { case 'json': if ('response' in xhr) { //IE does not support json as responseType, parse it internally - this.response = xhr.responseType ? xhr.response : JSON.parse(xhr.response || xhr.responseText || ''); + this.response = xhr.responseType ? xhr.response : JSON.parse(xhr.response || xhr.responseText || 'null'); } else { - this.response = JSON.parse(xhr.responseText || ''); + this.response = JSON.parse(xhr.responseText || 'null'); } break; case 'xml':