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.

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

fix(common): handle JS floating point errors in percent pipe #20329

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@ocombe
Contributor

ocombe commented Nov 10, 2017

PR Type

What kind of change does this PR introduce?

[x] Bugfix

What is the current behavior?

We multiply the number by 100 in the percent pipe which can lead to floating point errors with float numbers.

Issue Number: #20136

What is the new behavior?

We now shift the decimal separator without using math, which avoids the js floating point error.
We also remove trailing 0 when rounding if they are not required by the minimal number of fraction (see added tests).

Does this PR introduce a breaking change?

[x] No
@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 10, 2017

@vicb

I don't understand the issue we are trying to solve here.

@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Nov 10, 2017

Contributor

@vicb WE are trying to get around this: http://0.30000000000000004.com/

Contributor

ocombe commented Nov 10, 2017

@vicb WE are trying to get around this: http://0.30000000000000004.com/

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Nov 10, 2017

Contributor
  • Why are we trying to fix JS here and nowhere else ?,
  • This is a breaking change,
  • This change lacks docs,
  • Why do we have a generic expensive function while we are only trying to shift the coma position ?,
  • where is the code from ? (missing credits/license),
  • where are the tests ?,
  • are we sure that users expect this behavior rather than the regular JS behavior ?
Contributor

vicb commented Nov 10, 2017

  • Why are we trying to fix JS here and nowhere else ?,
  • This is a breaking change,
  • This change lacks docs,
  • Why do we have a generic expensive function while we are only trying to shift the coma position ?,
  • where is the code from ? (missing credits/license),
  • where are the tests ?,
  • are we sure that users expect this behavior rather than the regular JS behavior ?
@ocombe

This comment has been minimized.

Show comment
Hide comment
@ocombe

ocombe Nov 13, 2017

Contributor

Why are we trying to fix JS here and nowhere else ?,

This is the only place where we change the number value (by a multiplication or something else that would trigger this js problem)

This is a breaking change,

No, this is a fix because it removes an unexpected behavior that no one wants to get

This change lacks docs,

Added more comments on the function

Why do we have a generic expensive function while we are only trying to shift the coma position ?,

Updated the function to be less generic since we only use it here.
Here is a jsperf of different alternatives: https://jsperf.com/fix-js-floating-point-errors and this one is the fatest/smallest that I've found. If you have something better then don't hesitate but it's not like we're going to run it 100 000 times / change detection

where is the code from ? (missing credits/license),

Added link to the source in the comments of the function

where are the tests ?

In packages/common/test/pipes/number_pipe_spec.ts, see the files changed by the commit

are we sure that users expect this behavior rather than the regular JS behavior ?

We have a bug report about this and I tend to agree with it. Do you really expect to get 12.3456740000 % when you use the percent pipe like this {{ 0.12345674 | percent : '0.0-10' }} ?

Contributor

ocombe commented Nov 13, 2017

Why are we trying to fix JS here and nowhere else ?,

This is the only place where we change the number value (by a multiplication or something else that would trigger this js problem)

This is a breaking change,

No, this is a fix because it removes an unexpected behavior that no one wants to get

This change lacks docs,

Added more comments on the function

Why do we have a generic expensive function while we are only trying to shift the coma position ?,

Updated the function to be less generic since we only use it here.
Here is a jsperf of different alternatives: https://jsperf.com/fix-js-floating-point-errors and this one is the fatest/smallest that I've found. If you have something better then don't hesitate but it's not like we're going to run it 100 000 times / change detection

where is the code from ? (missing credits/license),

Added link to the source in the comments of the function

where are the tests ?

In packages/common/test/pipes/number_pipe_spec.ts, see the files changed by the commit

are we sure that users expect this behavior rather than the regular JS behavior ?

We have a bug report about this and I tend to agree with it. Do you really expect to get 12.3456740000 % when you use the percent pipe like this {{ 0.12345674 | percent : '0.0-10' }} ?

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 13, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 13, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 13, 2017

@@ -356,11 +363,19 @@ function roundNumber(parsedNumber: ParsedNumber, minFrac: number, maxFrac: numbe
// Pad out with zeros to get the required fraction length
for (; fractionLen < Math.max(0, fractionSize); fractionLen++) digits.push(0);

This comment has been minimized.

@vicb

vicb Dec 19, 2017

Contributor

digits.push(...new Array(Math.max(0. fractionSize)).fill(0));

@vicb

vicb Dec 19, 2017

Contributor

digits.push(...new Array(Math.max(0. fractionSize)).fill(0));

This comment has been minimized.

@ocombe

ocombe Dec 20, 2017

Contributor

it doesn't work because it doesn't test the value of fractionLen compared to Math.max(0, fractionSize)

@ocombe

ocombe Dec 20, 2017

Contributor

it doesn't work because it doesn't test the value of fractionLen compared to Math.max(0, fractionSize)

@vicb

vicb approved these changes Dec 20, 2017

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Dec 20, 2017

Contributor

LGTM once the CI is green. Great fix.
Please create an issue on AngularJS referencing this one.

Contributor

vicb commented Dec 20, 2017

LGTM once the CI is green. Great fix.
Please create an issue on AngularJS referencing this one.

IgorMinar added a commit that referenced this pull request Dec 22, 2017

@IgorMinar IgorMinar closed this in 07b81ae Dec 22, 2017

@ocombe ocombe deleted the ocombe:fix/#20136-percent-pipe branch Jan 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment