Skip to content

Commit

Permalink
fix(AjaxObservable): return null value from JSON.Parse (#1904)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
danmarshall authored and jayphelps committed Aug 31, 2016
1 parent 990594b commit 6ba374e
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 14 deletions.
56 changes: 45 additions & 11 deletions spec/helpers/ajax-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 = {
Expand All @@ -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.
Expand All @@ -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();
}

}
98 changes: 97 additions & 1 deletion spec/observables/dom/ajax-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
});

});
});
4 changes: 2 additions & 2 deletions src/observable/dom/AjaxObservable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down

0 comments on commit 6ba374e

Please sign in to comment.