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

Author: danmarshall

spec/helpers/ajax-helper.ts

@@ -117,17 +117,17 @@ export class MockXMLHttpRequest {
 
   private previousRequest: MockXMLHttpRequest;
 
-  private responseType: string = '';
+  protected responseType: string = '';
   private eventHandlers: Array<any> = [];
   private readyState: number = 0;
 
   private user: any;
   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 {
       }
     });
   }
-}
+}
+
+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();
+  }
+
+}

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;
+    });
+
   });
 });

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':