From 179fd739f5b7702e3cc59e0e6398973b714a8add Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Wed, 24 Aug 2016 14:15:14 -0700 Subject: [PATCH 1/7] 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. --- src/observable/dom/AjaxObservable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/observable/dom/AjaxObservable.ts b/src/observable/dom/AjaxObservable.ts index 84f815fbe1..02b7d2e82d 100644 --- a/src/observable/dom/AjaxObservable.ts +++ b/src/observable/dom/AjaxObservable.ts @@ -396,7 +396,7 @@ 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 || ''); } From c6e243479ec6389ce513b20c22a2fb5a8e2cf93a Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Thu, 25 Aug 2016 14:49:17 -0700 Subject: [PATCH 2/7] added ajax tests for 204 No Content --- spec/observables/dom/ajax-spec.ts | 61 +++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 444d146e36..bab1786ff6 100644 --- a/spec/observables/dom/ajax-spec.ts +++ b/spec/observables/dom/ajax-spec.ts @@ -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,38 @@ 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', + 'responseText': expected + }); + + expect(result.response).to.deep.equal(expected); + expect(complete).to.be.true; + }); + }); }); \ No newline at end of file From f66c83b10aea534a7b0ab3917d85b5b29ef87c39 Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Thu, 25 Aug 2016 18:41:38 -0700 Subject: [PATCH 3/7] mock the behavior of IE unit test for IE --- spec/helpers/ajax-helper.ts | 59 +++++++++++++++++++++++++------ spec/observables/dom/ajax-spec.ts | 39 ++++++++++++++++++-- 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/spec/helpers/ajax-helper.ts b/spec/helpers/ajax-helper.ts index 244a6e345d..ed0390c2a2 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,33 @@ export class MockXMLHttpRequest { } }); } -} \ No newline at end of file +} + +export class MockXMLHttpRequest_InternetExplorer extends MockXMLHttpRequest { + constructor() { + super(); + } + + 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 bab1786ff6..9eb8b2d72c 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, MockXMLHttpRequest_InternetExplorer} from '../../helpers/ajax-helper'; declare const global: any; @@ -555,10 +555,45 @@ describe('Observable.ajax', () => { request.respondWith({ 'status': 204, 'contentType': 'application/json', + 'responseType': 'json', 'responseText': expected }); - expect(result.response).to.deep.equal(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 = MockXMLHttpRequest_InternetExplorer; + + 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; }); From 0d1d3478959eac0513911693c7bec6bce622992c Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Thu, 25 Aug 2016 18:50:36 -0700 Subject: [PATCH 4/7] pascal case class name. added missing semicolon. --- spec/helpers/ajax-helper.ts | 4 ++-- spec/observables/dom/ajax-spec.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/helpers/ajax-helper.ts b/spec/helpers/ajax-helper.ts index ed0390c2a2..83c5216348 100644 --- a/spec/helpers/ajax-helper.ts +++ b/spec/helpers/ajax-helper.ts @@ -228,7 +228,7 @@ export class MockXMLHttpRequest { } } -export class MockXMLHttpRequest_InternetExplorer extends MockXMLHttpRequest { +export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest { constructor() { super(); } @@ -249,7 +249,7 @@ export class MockXMLHttpRequest_InternetExplorer extends MockXMLHttpRequest { protected defaultResponseValue() { if (this.status == 204) { - this.mockHttp204() + this.mockHttp204(); return; } return super.defaultResponseValue(); diff --git a/spec/observables/dom/ajax-spec.ts b/spec/observables/dom/ajax-spec.ts index 9eb8b2d72c..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, MockXMLHttpRequest_InternetExplorer} from '../../helpers/ajax-helper'; +import {MockXMLHttpRequest, MockXMLHttpRequestInternetExplorer} from '../../helpers/ajax-helper'; declare const global: any; @@ -568,7 +568,7 @@ describe('Observable.ajax', () => { let result: Rx.AjaxResponse; let complete = false; - root.XMLHttpRequest = MockXMLHttpRequest_InternetExplorer; + root.XMLHttpRequest = MockXMLHttpRequestInternetExplorer; Rx.Observable .ajax.post('/flibbertyJibbet', expected) From 6d87eb598537a26641a58a19ca07489aeba7e7e7 Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Mon, 29 Aug 2016 20:54:41 -0700 Subject: [PATCH 5/7] Use 'null' instead of '' for JSON.parse --- src/observable/dom/AjaxObservable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/observable/dom/AjaxObservable.ts b/src/observable/dom/AjaxObservable.ts index 0da4b8485f..9ae9916899 100644 --- a/src/observable/dom/AjaxObservable.ts +++ b/src/observable/dom/AjaxObservable.ts @@ -398,7 +398,7 @@ export class AjaxResponse { //IE does not support json as responseType, parse it internally 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': From f3b08c13437ae5bd9bdc471a5bd1cb9ea900ab50 Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Mon, 29 Aug 2016 21:13:25 -0700 Subject: [PATCH 6/7] remove empty constructor --- spec/helpers/ajax-helper.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/spec/helpers/ajax-helper.ts b/spec/helpers/ajax-helper.ts index 83c5216348..9c112c8635 100644 --- a/spec/helpers/ajax-helper.ts +++ b/spec/helpers/ajax-helper.ts @@ -229,9 +229,6 @@ export class MockXMLHttpRequest { } export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest { - constructor() { - super(); - } private mockHttp204() { this.responseType = ''; From 6670136fb87763ac06c5f5072bf5ccd02657cbac Mon Sep 17 00:00:00 2001 From: Dan Marshall Date: Wed, 24 Aug 2016 14:15:14 -0700 Subject: [PATCH 7/7] 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':