Return null value from JSON.Parse #1904

danmarshall · 2016-08-24T21:15:33Z

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.

kwonoj · 2016-08-24T21:16:48Z

Would you add test cases for this change as well?

coveralls · 2016-08-24T21:20:37Z

Coverage remained the same at 97.115% when pulling 179fd73 on danmarshall:patch-1 into 80b817f on ReactiveX:master.

danmarshall · 2016-08-24T21:50:49Z

Where do you add your tests?

kwonoj · 2016-08-24T21:52:35Z

I believe ajax-spec is place to go.

danmarshall · 2016-08-24T22:58:25Z

Hi @kwonoj - I cloned locally and tried doing a build, to get myself familiar with the test runner. At some point it failed when it tried to run java (which I don't have) so I think the level of setup is greater than the code contribution... what do you think?

kwonoj · 2016-08-24T23:02:32Z

Jre is only needed to generate global build so you can safely ignore if you do not like to install jre. Just build / test would be sufficient for general workflow such as npm run build_test. You may able to check npm run info for other tasks available.

danmarshall · 2016-08-25T19:49:22Z

@kwonoj do you have a test for your original fix that I can use as an example?

kwonoj · 2016-08-25T19:54:35Z

No, I just noticed I skipped test cases and just verified manually. Seems that's reason I wasn't able to caught issue, test didn't cover it.

coveralls · 2016-08-25T22:25:28Z

Coverage remained the same at 97.115% when pulling 3669375 on danmarshall:patch-1 into 80b817f on ReactiveX:master.

unit test for IE

added missing semicolon.

coveralls · 2016-08-26T01:55:48Z

Coverage remained the same at 97.115% when pulling 0d1d347 on danmarshall:patch-1 into 80b817f on ReactiveX:master.

danmarshall · 2016-08-26T02:01:15Z

I verified that this unit test fails on the master branch without the fix, passes on this branch with the fix.

coveralls · 2016-08-26T17:50:20Z

Coverage remained the same at 97.115% when pulling fe61805 on danmarshall:patch-1 into 7b23e88 on ReactiveX:master.

danmarshall · 2016-08-26T18:09:42Z

Hi @kwonoj would you mind taking a look at this? I spent a good deal of time yesterday writing the unit test, wanted to make sure that my additions are compatible with your framework style standards.

kwonoj · 2016-08-26T18:22:33Z

Sure, please allow me some time as I'll be offline for some time. I've brifely looked changes, interested if it's possible to remove mock for IE only - but that'll need some trial on my end.

danmarshall · 2016-08-26T19:01:49Z

Mocking {x} is the way to test {x}, so mocking IE would be the way to test IE :)

kwonoj · 2016-08-26T19:04:24Z

That I agree completely, just about high level approach. I'll comment in detail soon as I promised.

kwonoj · 2016-08-30T03:25:00Z

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 || '');


isn't this JSON.parse(xhr.responseText || ''); also need to be updated to 'null' for fallback string? this isn't directly related to IE, but still seems possible unexpected failure cases.

Do you mean on line 401 ? I did not exercise that code path, but I suppose that yes it should.

yes, 401. I know it's not directly related (for execution path to IE), but seems need to be updated as well.

coveralls · 2016-08-30T03:59:09Z

Coverage remained the same at 97.115% when pulling 6d87eb5 on danmarshall:patch-1 into 7b23e88 on ReactiveX:master.

coveralls · 2016-08-30T04:09:59Z

Coverage remained the same at 97.115% when pulling 3c661b5 on danmarshall:patch-1 into cb40e92 on ReactiveX:master.

kwonoj · 2016-08-30T04:11:26Z

spec/helpers/ajax-helper.ts

+
+export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest {
+  constructor() {


you can skip ctor if it doesn't do specific thing in inherited.

coveralls · 2016-08-30T04:18:01Z

Coverage remained the same at 97.115% when pulling f3b08c1 on danmarshall:patch-1 into cb40e92 on ReactiveX:master.

kwonoj · 2016-08-30T04:26:09Z

LGTM, I'll leave this PR around couple of days to see if other suggestions around before check in.

jayphelps · 2016-08-31T05:48:48Z

LGTM, needs rebase plz and while you're add it please squash as a single commit using our commit message format (see existing commits for reference)

For others reference, the behavior of having it be xhr.response = null is per spec when responseType === 'json'

https://xhr.spec.whatwg.org/#json-response

coveralls · 2016-08-31T06:14:37Z

Coverage remained the same at 97.115% when pulling 691dd2e on danmarshall:patch-1 into 990594b on ReactiveX:master.

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

…tch-1

coveralls · 2016-08-31T07:23:02Z

Coverage remained the same at 97.115% when pulling fe61805 on danmarshall:patch-1 into 7b23e88 on ReactiveX:master.

jayphelps · 2016-08-31T17:35:45Z

@danmarshall I went ahead and squashed it. Check out http://push.cwcon.org/learn/squashing-commits.html for further info about how to squash your commits into a one for future PRs. 💃

Thanks for your fix!

danmarshall · 2016-08-31T17:59:15Z

Awesome! When is your next publish to NPM ?

jayphelps · 2016-08-31T18:56:32Z

@danmarshall the next beta release isn't set. There are usually a release at least once a month, so we're due for one shortly I imagine. It's up to @Blesh

lock · 2018-06-06T20:41:47Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Return null value from JSON.Parse

179fd73

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.

danmarshall added 2 commits August 25, 2016 14:49

added ajax tests for 204 No Content

c6e2434

Merge branch 'master' into patch-1

3669375

danmarshall added 2 commits August 25, 2016 18:41

mock the behavior of IE

f66c83b

unit test for IE

pascal case class name.

0d1d347

added missing semicolon.

Merge branch 'master' into patch-1

fe61805

kwonoj reviewed Aug 30, 2016
View reviewed changes

Use 'null' instead of '' for JSON.parse

6d87eb5

Merge branch 'master' into patch-1

3c661b5

kwonoj reviewed Aug 30, 2016
View reviewed changes

remove empty constructor

f3b08c1

kwonoj added the PR: needs rebase label Aug 31, 2016

Merge branch 'master' into patch-1

691dd2e

danmarshall added 2 commits August 31, 2016 00:12

fix(AjaxObservable): return null value from JSON.Parse

6670136

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

Merge branch 'patch-1' of https://github.com/danmarshall/rxjs into pa…

4e15603

…tch-1

jayphelps merged commit 6ba374e into ReactiveX:master Aug 31, 2016

jayphelps removed the PR: needs rebase label Aug 31, 2016

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return null value from JSON.Parse #1904

Return null value from JSON.Parse #1904

danmarshall commented Aug 24, 2016

kwonoj commented Aug 24, 2016

coveralls commented Aug 24, 2016

danmarshall commented Aug 24, 2016

kwonoj commented Aug 24, 2016

danmarshall commented Aug 24, 2016

kwonoj commented Aug 24, 2016

danmarshall commented Aug 25, 2016

kwonoj commented Aug 25, 2016

coveralls commented Aug 25, 2016

coveralls commented Aug 26, 2016

danmarshall commented Aug 26, 2016

coveralls commented Aug 26, 2016

danmarshall commented Aug 26, 2016

kwonoj commented Aug 26, 2016

danmarshall commented Aug 26, 2016

kwonoj commented Aug 26, 2016

kwonoj Aug 30, 2016

danmarshall Aug 30, 2016

kwonoj Aug 30, 2016

coveralls commented Aug 30, 2016

coveralls commented Aug 30, 2016

kwonoj Aug 30, 2016

coveralls commented Aug 30, 2016

kwonoj commented Aug 30, 2016

jayphelps commented Aug 31, 2016

coveralls commented Aug 31, 2016

coveralls commented Aug 31, 2016 •

edited

Loading

jayphelps commented Aug 31, 2016 •

edited

Loading

danmarshall commented Aug 31, 2016

jayphelps commented Aug 31, 2016

lock bot commented Jun 6, 2018


		export class MockXMLHttpRequestInternetExplorer extends MockXMLHttpRequest {
		constructor() {

Return null value from JSON.Parse #1904

Return null value from JSON.Parse #1904

Conversation

danmarshall commented Aug 24, 2016

kwonoj commented Aug 24, 2016

coveralls commented Aug 24, 2016

danmarshall commented Aug 24, 2016

kwonoj commented Aug 24, 2016

danmarshall commented Aug 24, 2016

kwonoj commented Aug 24, 2016

danmarshall commented Aug 25, 2016

kwonoj commented Aug 25, 2016

coveralls commented Aug 25, 2016

coveralls commented Aug 26, 2016

danmarshall commented Aug 26, 2016

coveralls commented Aug 26, 2016

danmarshall commented Aug 26, 2016

kwonoj commented Aug 26, 2016

danmarshall commented Aug 26, 2016

kwonoj commented Aug 26, 2016

kwonoj Aug 30, 2016

Choose a reason for hiding this comment

danmarshall Aug 30, 2016

Choose a reason for hiding this comment

kwonoj Aug 30, 2016

Choose a reason for hiding this comment

coveralls commented Aug 30, 2016

coveralls commented Aug 30, 2016

kwonoj Aug 30, 2016

Choose a reason for hiding this comment

coveralls commented Aug 30, 2016

kwonoj commented Aug 30, 2016

jayphelps commented Aug 31, 2016

coveralls commented Aug 31, 2016

coveralls commented Aug 31, 2016 • edited Loading

jayphelps commented Aug 31, 2016 • edited Loading

danmarshall commented Aug 31, 2016

jayphelps commented Aug 31, 2016

lock bot commented Jun 6, 2018

coveralls commented Aug 31, 2016 •

edited

Loading

jayphelps commented Aug 31, 2016 •

edited

Loading