Fixing JSON decoding for Ints.

[krisajenkins]

One way to check if a number is an integer in JavaScript is (value|0).
Sadly, this fails if the value is greater than 2^32, but still less than
Number.MAX_SAFE_INT.

This patch switches the Json decoder to use a polyfill of the proposed
ES6 Number.isInteger function.

[krisajenkins]

Oops, sorry - that should read Number.MAX_SAFE_INTEGER.

[krisajenkins]

Oops, sorry - that should read Number.MAX_SAFE_INTEGER.

[krisajenkins]

This bug has just bitten me again, which in turn has nudged me to rebase against the current master branch. :-)

[krisajenkins]

This bug has just bitten me again, which in turn has nudged me to rebase against the current master branch. :-)

[ajhager]

I have benchmarked and unit tested the original code, your proposed change, and a new method of my own design (original, proposed, and blend respectively below.) I used the tests from https://github.com/parshap/js-is-integer/blob/master/test.js and the benchmarks are at http://jsperf.com/isinteger-elm.

The original does indeed fail for values over and under 2147483647. proposed and blend are both correct, but blend is only slightly slower than the original in the general case and nearly equivalent in the typical case.

function original(value) {
  return typeof value === 'number' && (value|0) === value
}

function proposed(value) {
  return typeof value === "number" && isFinite(value) && value > -9007199254740992 && value < 9007199254740992 && Math.floor(value) === value;
}

function blend(value) {
  if (typeof value !== 'number') {
    return false;
  }

  if (value < 2147483647 && value > -2147483647) {
    return (value|0) === value;
  }

  return isFinite(value) && !(value % 1);
}    

No matter which function we go with, there needs to be some JSON tests to cover this. I would be happy to add those though.

[ajhager]

I have benchmarked and unit tested the original code, your proposed change, and a new method of my own design (original, proposed, and blend respectively below.) I used the tests from https://github.com/parshap/js-is-integer/blob/master/test.js and the benchmarks are at http://jsperf.com/isinteger-elm.

The original does indeed fail for values over and under 2147483647. proposed and blend are both correct, but blend is only slightly slower than the original in the general case and nearly equivalent in the typical case.

function original(value) {
  return typeof value === 'number' && (value|0) === value
}

function proposed(value) {
  return typeof value === "number" && isFinite(value) && value > -9007199254740992 && value < 9007199254740992 && Math.floor(value) === value;
}

function blend(value) {
  if (typeof value !== 'number') {
    return false;
  }

  if (value < 2147483647 && value > -2147483647) {
    return (value|0) === value;
  }

  return isFinite(value) && !(value % 1);
}    

No matter which function we go with, there needs to be some JSON tests to cover this. I would be happy to add those though.

[evancz]

@krisajenkins, thanks for providing a fix for this! I was not sure whether the behavior you expect is the behavior everyone would expect. If @ajhager agrees, I guess there is some sort of consensus :)

I'm trying this new thing out where I close issues more aggressively, even if they are a good idea that should happen. In this case, it seems like there are multiple folks who can do the updated fix, so to avoid a chance of blocking, can we do a new PR with the faster version and some tests?

[evancz]

@krisajenkins, thanks for providing a fix for this! I was not sure whether the behavior you expect is the behavior everyone would expect. If @ajhager agrees, I guess there is some sort of consensus :)

I'm trying this new thing out where I close issues more aggressively, even if they are a good idea that should happen. In this case, it seems like there are multiple folks who can do the updated fix, so to avoid a chance of blocking, can we do a new PR with the faster version and some tests?

[ajhager]

I will do the new PR with tests.

[ajhager]

I will do the new PR with tests.

[evancz]

Awesome, thank you!

Also, sorry for the huge delay @krisajenkins, and thank you again @ajhager for unsticking things! :)

[evancz]

Awesome, thank you!

Also, sorry for the huge delay @krisajenkins, and thank you again @ajhager for unsticking things! :)

[krisajenkins]

Cool, this'll be great to see. I'm decoding some JSON with some large productIDs, and having to manually meddle with elm-stuff keeps me awake at night. :-D

[krisajenkins]

Cool, this'll be great to see. I'm decoding some JSON with some large productIDs, and having to manually meddle with elm-stuff keeps me awake at night. :-D