fix(input, input-number): increment/decrement unsafe numbers without loss of precision #6580

benelan · 2023-03-09T00:21:10Z

Related Issue: #5920

Summary

Numbers that cannot be represented in JavaScript's IEEE-754 double precision were being rounded when incrementing/decrementing because they were cast as Number (we store the value as a string). We use BigDecimal class to support numbers that do not fit into JavaScript's encoding method. I had overlooked the increment/decrement functionality when implementing BigDecimal.

BigDecimal uses BigInt internally, and unfortunately BigInt doesn't support exponential notation. For example, BigInt("1e2") throws the error:

Uncaught SyntaxError: invalid BigInt syntax

To work around the constraint, I manually expand any exponential notation numbers into normal decimal numbers. All of this is done via string manipulation, so sorry in advance for the burned retinas you are about to receive from this PR review.

For more info:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger#description

…ment-bigdecimal * origin/master: test(tabs): delay story due to potential timing chromatic diff (#6437) build(deps): Bump chromatic from 6.14.0 to 6.17.1 (#6571) build(deps): Bump postcss from 8.4.20 to 8.4.21 (#6570) build(deps): Bump eslint-config-prettier from 8.6.0 to 8.7.0 (#6572) ci(eslint): ignore private/internal code for jsdoc rules (#6416)

src/utils/number.ts

jcfranco

✨🔢🔧✨

jcfranco · 2023-03-10T17:16:09Z

src/components/input-number/input-number.tsx

@@ -751,7 +761,7 @@ export class InputNumber
    event.stopPropagation();
  };

-  private setChildNumberElRef = (el) => {
+  private setChildNumberElRef = (el: HTMLInputElement) => {


jcfranco · 2023-03-10T17:18:28Z

src/utils/math.ts

+const decimalNumberRegex = new RegExp(/(?:\.(\d+))?(?:[eE]([+-]?\d+))?$/);
+
+export const decimalPlaces = (value: string): number => {
+  const match = value.match(decimalNumberRegex);


Suggestion: would test help simplify this?

We need to use the values of the match a couple lines down

jcfranco · 2023-03-10T17:22:41Z

src/components/tooltip/tooltip.e2e.ts

@@ -12,6 +12,8 @@ describe("calcite-tooltip", () => {
  /**
   * Helps assert the canceled Esc key press when closing tooltips
   * Must be called before the tooltip is closed via keyboard.
+   *
+   * @param page


Can you roll this file back? I think it was auto-fixed by the linter, but the file is unrelated to this PR.

jcfranco · 2023-03-10T17:25:09Z

src/components/slider/slider.tsx

@@ -1200,7 +1200,7 @@ export class Slider
    const percent = (x - left) / width;
    const mirror = this.shouldMirror();
    const clampedValue = this.clamp(this.min + range * (mirror ? 1 - percent : percent));
-    let value = Number(clampedValue.toFixed(decimalPlaces(this.step)));
+    let value = Number(clampedValue.toFixed(decimalPlaces(`${this.step}`)));


If this fixes precision on the slider, can you update the PR title to reflect this? If it's to fix types, can you submit this in a separate PR? If it's the latter, we can merge the separate PR first and we can skip rolling it back here.

The parameter needs to be a string so the decimalPlaces function can also be used with BigDecimal. Previously step was being converted to a string in the decimalPlaces function itself, but we would lose precision if converting BigDecimal to number before passing it to the function. So it doesn't fix anything in slider, it's just a small, required refactor.

Nevermind, Input isn't using decimalPlaces anymore, removing!

jcfranco · 2023-03-10T17:29:54Z

src/utils/number.ts

+  if (exponentialParts.length === 1) {
+    return numberString;
+  }
+  if (Number.isSafeInteger(+numberString)) {


Nitpick: for readability, can you add a newline between if statements?

jcfranco · 2023-03-10T17:30:54Z

src/utils/number.ts

+  if (exponentialParts.length === 1) {
+    return numberString;
+  }
+  if (Number.isSafeInteger(+numberString)) {


Can you store +numberString in a local variable and reuse it?

jcfranco · 2023-03-10T17:35:39Z

src/components/input/input.tsx


-    const inputValPlaces = decimalPlaces(inputVal);
-    const inputStepPlaces = decimalPlaces(inputStep);
+    const finalValue = nudgedValueBelowInputMin


Suggestion: can this be refactored to compute and evaluate a nudged value check at a time? It's currently computing two values where one might possibly be thrown away.

…ment-bigdecimal * origin/master: build(deps): Bump rimraf and @types/rimraf (#6541) build(deps): Bump quicktype-core from 6.1.0 to 23.0.12 (#6573)

…calcite-components into benelan/input-increment-bigdecimal * 'benelan/input-increment-bigdecimal' of github.com:esri/calcite-components: chore: cleanup fix: remove BigDecimal.toFixed() method style: format

benelan and others added 8 commits November 30, 2022 13:07

fix(input-number): does not lose precision when nudged

09e8307

chore: fix merge conflict

4ab1bd5

fix: toFixed error due to calling toString on this.value instead of this

81cdff0

chore: manually convert exponential notation to decimal format

f659eb9

all existing tests passing

e3c746f

chore: [WIP] 2023-03-08T13:23:43-08:00

da95105

got it working and added the code to input

e7897be

benelan requested a review from a team as a code owner March 9, 2023 00:21

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Mar 9, 2023

github-actions bot assigned benelan Mar 9, 2023

github-advanced-security bot found potential problems Mar 9, 2023

View reviewed changes

src/utils/number.ts Fixed Show fixed Hide fixed

src/utils/number.ts Fixed Show fixed Hide fixed

src/utils/number.ts Fixed Show resolved Hide resolved

benelan added 3 commits March 8, 2023 16:25

style: format

c062ee3

fix: remove BigDecimal.toFixed() method

b4f7544

chore: cleanup

1a95696

jcfranco approved these changes Mar 10, 2023

View reviewed changes

benelan added 4 commits March 13, 2023 11:20

Merge remote-tracking branch 'origin/master' into benelan/input-incre…

7c2ac43

…ment-bigdecimal * origin/master: build(deps): Bump rimraf and @types/rimraf (#6541) build(deps): Bump quicktype-core from 6.1.0 to 23.0.12 (#6573)

chore: review cleanup

0c2e026

cleanup

19ca244

benelan added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 13, 2023

benelan merged commit 40c0f0f into master Mar 14, 2023

benelan deleted the benelan/input-increment-bigdecimal branch March 14, 2023 00:00

github-actions bot added this to the 2023 March Priorities - No breaking changes milestone Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(input, input-number): increment/decrement unsafe numbers without loss of precision #6580

fix(input, input-number): increment/decrement unsafe numbers without loss of precision #6580

Uh oh!

benelan commented Mar 9, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcfranco left a comment

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

benelan Mar 13, 2023

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

benelan Mar 13, 2023

Uh oh!

benelan Mar 13, 2023

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

jcfranco Mar 10, 2023

Uh oh!

Uh oh!

fix(input, input-number): increment/decrement unsafe numbers without loss of precision #6580

fix(input, input-number): increment/decrement unsafe numbers without loss of precision #6580

Uh oh!

Conversation

benelan commented Mar 9, 2023

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcfranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!