[Bug Fix: 15635] Fix Displaying of number literals bigger than 9 digits

[saravanak]

Fixes #15635.

What:
As suggested here #15635 (comment),
we transform the AST for big number literals into (-parse-int "1546416000000").

I've used String(number).length > 9 to identify cases where we need to do the transform.

Added tests for the AST Transform, and the new internal -parse-int helper.

@chancancode Can you have a look at this?

I am not sure if the target branch is right though...

Need some pointers on

• Updating the guides (This is internal, so imho, we don't need to update the documentation)

[Mooninaut]

Needs to use length > 8 The bug can occur with values >= 536870912 and < 1000000000. For example, 678881280 is displayed as 142010368.

[chancancode]

I think we should just parse the number with parseInt and test the actual value, instead of the length of the string

[chancancode]

We need to check for floats as well, maybe just use parseFloat here, check that it is an integer (parseFloat(...) % 1 === 0?) and is out of bounds, both the upper and lower (negative) bounds.

[lifeart]

or new Number('1').valueOf()or +'12'

[chancancode]

Suggested change

	function parseIntHelper({ positional }: any) {
	function parseIntHelper({ positional }: CapturedArguments) {

You'll have to import CapturedArguments from @glimmer/interfaces

[chancancode]

Suggested change

	return parseInt(value);
	return parseInt(value, 10);

[chancancode]

Instead of having a unit test for the helper, I would prefer that we have an acceptance/integration test for being able to render/pass big numbers.

[chancancode]

https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/glimmer/tests/integration/content-test.js would probably be a good place for it

[chancancode]

Can you add a few more test for things like {{1234567890}}, {{some-helper (another-helper 1234567890)}}, <Foo @bar={{1234567890}} />, {{some-helper foo=1234567890}}, etc?

[chancancode]

Overall looks good to me, left some comments for things that need to be fixed before merging

[saravanak]

Overall looks good to me, left some comments for things that need to be fixed before merging

@chancancode @lifeart @Mooninaut,

Thanks for the comments...

Given that it was an old bug, I should have checked the replication before attempting the PR; my bad.

Here are the results though:

From the twiddle shared by @rwjblue in the bug description, I had

{{go-boom-for-big-numbers 1234567893}}
{{go-boom-for-big-numbers 12345678933}}
<div/>
{{go-boom-for-big-numbers 1234567893232323.2323}}
{{go-boom-for-big-numbers 1234567893323232323.2323}}
<div/>
{{go-boom-for-big-numbers -1234567893232323.2323}}
{{go-boom-for-big-numbers -1234567893323232323.2323}}
<div/>
{{go-boom-for-big-numbers -1234567893}}
{{go-boom-for-big-numbers -12345678933}}

The following are the results:

Ember 3.0.0
(No display, console error) : Uncaught TypeError: reference$$1.value is not a function

Ember 3.1.4 and newer

160826069 12345678933
1234567893232323.2 1234567893323232300
-1234567893232323.2 -1234567893323232300
-160826069 -12345678933

There is also an error in the 3.1.4 results: 1234567893 is displayed as 160826069. I think this should be treated as a glimmer-vm bug and tracked separately.

Looks like this is fixed 'upstream' in newer versions of glimmer-vm. I was not able to locate the actual diff that fixed this in the glimmer-vm code.

This makes this PR obsolete, as the relevant fixes are done on the glimmer-vm code.

I'd like a go-ahead and on continuation of this PR if its still needed..

[chancancode]

Rather than a test for the transform, maybe we should just have a set of runtime tests like this:

let i = Number.MAX_SAFE_INTEGER;

while (i >= 1) {
  let number = i;

  test(`it can handle ${number}`, function() {
    this.registerHelper('is-equal-integer', function([actual, expected]) {
      return typeof actual === 'number' && actual === expected;
    });

    this.render(`[{{${number}}}][{{this.number}}][{{is-equal-integer ${number} this.number}}]`, { number });

    this.assertText(`[${number}][${number}][true]`);
  });

  i = Math.round(i / 2);
};

let j = Number.MIN_SAFE_INTEGER;

while (j <= -1) {
  let number = j;

  test(`it can handle ${number}`, function() {
    this.registerHelper('is-equal-integer', function([actual, expected]) {
      return typeof actual === 'number' && actual === expected;
    });

    this.render(`[{{${number}}}][{{this.number}}][{{is-equal-integer ${number} this.number}}]`, { number });

    this.assertText(`[${number}][${number}][true]`);
  });

  j = Math.round(j / 2);
};