Skip to content
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 floating-point arithmetic issues when props are decimals. #619

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 38 additions & 13 deletions src/utils.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { findDOMNode } from 'react-dom';
import keyCode from 'rc-util/lib/KeyCode';

const MAX_PRECISION_FOR_OPERATIONS = 15;

export function isEventFromHandle(e, handles) {
try {
return Object.keys(handles)
Expand All @@ -19,28 +21,51 @@ export function isNotTouchEvent(e) {
(e.type.toLowerCase() === 'touchend' && e.touches.length > 0);
}

export function getPrecision(step) {
const stepString = step.toString();
let precision = 0;
if (stepString.indexOf('.') >= 0) {
precision = stepString.length - stepString.indexOf('.') - 1;
}
return precision;
}
Comment on lines +24 to +31
Copy link
Author

Choose a reason for hiding this comment

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

This function is untouched, moved it up because I'm using it elsewhere.


function withPrecision(value, precision) {
return parseFloat(value.toFixed(precision));
}

// safeDivideBy and safeMultiply: if either term is a float,
// then round the result to the combined precision

function safeDivideBy(a, b) {
const precision = Math.min(
getPrecision(a) + getPrecision(b),
MAX_PRECISION_FOR_OPERATIONS
);
return precision === 0 ? a / b : withPrecision(a / b, precision);
}

function safeMultiply(a, b) {
const precision = Math.min(
getPrecision(a) + getPrecision(b),
MAX_PRECISION_FOR_OPERATIONS
);
return withPrecision(a * b, precision);
}

export function getClosestPoint(val, { marks, step, min, max }) {
const points = Object.keys(marks).map(parseFloat);
if (step !== null) {
const maxSteps = Math.floor((max - min) / step);
const steps = Math.min((val - min) / step, maxSteps);
const maxSteps = Math.floor(safeDivideBy(max - min, step));
const steps = Math.min(safeDivideBy(val - min, step), maxSteps);
const closestStep =
Math.round(steps) * step + min;
safeMultiply(Math.round(steps), step) + min;
points.push(closestStep);
}
const diffs = points.map(point => Math.abs(val - point));
return points[diffs.indexOf(Math.min(...diffs))];
}

export function getPrecision(step) {
const stepString = step.toString();
let precision = 0;
if (stepString.indexOf('.') >= 0) {
precision = stepString.length - stepString.indexOf('.') - 1;
}
return precision;
}

export function getMousePosition(vertical, e) {
return vertical ? e.clientY : e.pageX;
}
Expand Down Expand Up @@ -70,7 +95,7 @@ export function ensureValuePrecision(val, props) {
const { step } = props;
const closestPoint = isFinite(getClosestPoint(val, props)) ? getClosestPoint(val, props) : 0; // eslint-disable-line
return step === null ? closestPoint :
parseFloat(closestPoint.toFixed(getPrecision(step)));
withPrecision(closestPoint, getPrecision(step));
Comment on lines -73 to +98
Copy link
Author

@jcfrancisco jcfrancisco Jan 21, 2020

Choose a reason for hiding this comment

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

I can back out this refactor if it's unwanted

}

export function pauseEvent(e) {
Expand Down
12 changes: 12 additions & 0 deletions tests/utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,17 @@ describe('utils', () => {

expect(utils.getClosestPoint(value, props)).toBe(96);
});

it('should properly handle floating point arithmetic', () => {
const value = 5.3;
const props = {
marks: {},
step: 0.05,
min: 0,
max: 5.3
};

expect(utils.getClosestPoint(value, props)).toBe(5.3);
});
});
});