Skip to content

Return null value from JSON.Parse #1904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Aug 31, 2016
Merged

Return null value from JSON.Parse #1904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
179fd73
Return null value from JSON.Parse
danmarshall Aug 24, 2016
c6e2434
added ajax tests for 204 No Content
danmarshall Aug 25, 2016
3669375
Merge branch 'master' into patch-1
danmarshall Aug 25, 2016
f66c83b
mock the behavior of IE
danmarshall Aug 26, 2016
0d1d347
pascal case class name.
danmarshall Aug 26, 2016
fe61805
Merge branch 'master' into patch-1
danmarshall Aug 26, 2016
6d87eb5
Use 'null' instead of '' for JSON.parse
danmarshall Aug 30, 2016
3c661b5
Merge branch 'master' into patch-1
danmarshall Aug 30, 2016
f3b08c1
remove empty constructor
danmarshall Aug 30, 2016
691dd2e
Merge branch 'master' into patch-1
danmarshall Aug 31, 2016
6670136
fix(AjaxObservable): return null value from JSON.Parse
danmarshall Aug 24, 2016
4e15603
Merge branch 'patch-1' of https://github.com/danmarshall/rxjs into pa…
danmarshall Aug 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 45 additions & 11 deletions spec/helpers/ajax-helper.ts
View file
Open in desktop
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() {
Copy link
Member

@kwonoj kwonoj Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't jsonResponseValue handles status 204? is there possibility to fall back into this overrided method?

Copy link
Contributor Author

@danmarshall danmarshall Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On other platforms, yes. This is the override to behave as IE

Copy link
Member

@kwonoj kwonoj Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pulled down and while trying it, seems this specific (MockXMLHttpRequestInternetExplorer::defaultResponseValue()) hasn't executed. Maybe I'm missing something?

Copy link
Contributor Author

@danmarshall danmarshall Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a scenario where one can write a unit test like this:

request.respondWith({
  'status': 204,
  'contentType': 'application/json',
  'responseType': 'json',
  'responseText': expected
});

The issue is that the IE does not actually provide 'responseType': 'json', so this would be a presumptuous test. That is why when we mock IE that we actually unset responseType in private mockHttp204()

That is the reason the MockXMLHttpRequestInternetExplorer overrides both, just in case.

Copy link
Member

@kwonoj kwonoj Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is why when we mock IE that we actually unset responseType in private mockHttp204()

: yes, that I'm aware of.

My question in here is, if overrided defaultResponseValue doesn't do any roles in inherited MockXMLHttpRequestIE, is it necessarily need to do it? test for behavior of IE is fulfilled by response override at line 242, so default case may not be separated / overrideded but just leave as-is to throw directly in line 199 instead. Of course it's for 'just in case' and to be possibly served in further though, I'm seeing inherited class can be simplified.

Copy link
Contributor Author

@danmarshall danmarshall Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if it is used incorrectly, (test respondWith 'responseType': 'json',) this will give a false positive. Basically, my intention was to keep all IE mock behavior inside of the class. Otherwise every test writer has to know how IE behaves.

Copy link
Member

@kwonoj kwonoj Aug 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks for clarification. I don't see it as hard blocking issue.

if (this.status == 204) {
this.mockHttp204();
return;
}
return super.defaultResponseValue();
}

}
98 changes: 97 additions & 1 deletion spec/observables/dom/ajax-spec.ts
View file
Open in desktop
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
View file
Open in desktop
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