Repository: amzn/ion-js (ts)

Issue:
DOM Decimal: super constructor call needs update (and looks buggy)
See [here](https://github.com/amzn/ion-js/blob/ada4439df48b0467c26ce6487bed78f050716360/src/dom/Decimal.ts#L34):
```
  constructor(value: ion.Decimal, annotations: string[] = []) {
    super(...[value.getCoefficient(), value.getExponent(), value.isNegative()]);
    this._decimalValue = value;
    this._numberValue = value.numberValue();
    this._setAnnotations(annotations);
  }
```
Since `class Decimal extends Value(Number, ...)`, `super` is effectively the plain JavaScript `Number` constructor. There are two issues here:

(1) `value.getCoefficient()` returns a JSBI instance. Newer versions of JSBI disallow `new Number(some_jsbi)` constructor calls. The supported alternative is to use `JSBI.toNumber(some_jsbi)` instead. This came up at https://github.com/GoogleChromeLabs/jsbi/issues/85.
(Background: this change was made in an effort to disallow invalid conversions that wouldn't work any more once JSBI is compiled away to native BigInts. `Number(42n)` is fine, but `+42n` is not; so JSBI now disallows `+some_jsbi`, which as "collateral damage" also disallows `Number(some_jsbi)`. FWIW, JSBI's [documentation](https://github.com/GoogleChromeLabs/jsbi#how) has always recommended to use `JSBI.toNumber`, the fact that `Number(some_jsbi)` also worked was accidental.)

(2) The `Number` constructor takes only one argument. Calling it with three arguments looks like a bug, as `value.getExponent()` and `value.isNegative()` will just be ignored. (Presumably the value passed to the super ctor is never used for anything, which would explain why this hasn't caused issues so far?)

I would suggest to fix both issues at once by changing the super call to `super(...[value.numberValue()])`. (Or, more simply, `super(value.numberValue())`. Or, if my suspicion is correct that the value is indeed never used, just drop the super call entirely.)

Apologies in advance if I've misunderstood anything.

Pull request:
Updates ion-js to be compatible with jsbi@^4.0.0
**Issue #, if available:**

Fixes #700 

**Description of changes:**

Updates usage of JSBI so that consumers of `ion-js` can use JSBI 3 or 4. JSBI 4 no longer allows certain built-in operators to be used with JSBI, and disallows new Number(some_jsbi) constructor calls. (See https://github.com/GoogleChromeLabs/jsbi/issues/85 and #700 for more details.) 

Anywhere that a JSBI was being coerced to a `Number` or a `string` was replaced with an explicit conversion (using `toString()`, `toNumber()`, or a template string). In the `Decimal` constructor, there was an indirect invocation of `new Number(some_jsbi)` that was updated to use `JSBI.toNumber()` instead.

*By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.*


Patch:
diff --git a/package.json b/package.json
index a811efc..2d53cd0 100644
--- a/package.json
+++ b/package.json
@@ -7,7 +7,9 @@
   "scripts": {
     "commit": "git-cz",
     "prepare": "grunt release",
-    "test": "nyc mocha",
+    "test": "npm run test-jsbi4 && npm run test-jsbi3",
+    "test-jsbi3": "npm install jsbi@3.1.1 && nyc mocha",
+    "test-jsbi4": "npm install jsbi@4.0.0 && nyc mocha",
     "release": "grunt release",
     "test-driver": "cd test-driver && npm install",
     "build-test-driver": "cd test-driver && npm run build",
diff --git a/src/IonTextWriter.ts b/src/IonTextWriter.ts
index d1d80d0..62a1c80 100644
--- a/src/IonTextWriter.ts
+++ b/src/IonTextWriter.ts
@@ -171,7 +171,7 @@ export class TextWriter extends AbstractWriter {
       const scale = -exponent;
 
       if (exponent == 0) {
-        s += coefficient + ".";
+        s += coefficient.toString() + ".";
       } else if (exponent < 0) {
         // Avoid printing small negative exponents using a heuristic
         // adapted from http://speleotrove.com/decimal/daconvs.html
@@ -186,15 +186,15 @@ export class TextWriter extends AbstractWriter {
         } else if (adjustedExponent >= -6) {
           s += "0.";
           s += "00000".substring(0, scale - significantDigits);
-          s += coefficient;
+          s += coefficient.toString();
         } else {
-          s += coefficient;
+          s += coefficient.toString();
           s += "d-";
           s += scale.toString();
         }
       } else {
         // exponent > 0
-        s += coefficient + "d" + exponent;
+        s += coefficient.toString() + "d" + exponent;
       }
 
       this.writeUtf8(s);
diff --git a/src/dom/Decimal.ts b/src/dom/Decimal.ts
index 5c0c996..160169a 100644
--- a/src/dom/Decimal.ts
+++ b/src/dom/Decimal.ts
@@ -31,7 +31,7 @@ export class Decimal extends Value(
    * @param annotations   An optional array of strings to associate with `value`.
    */
   constructor(value: ion.Decimal, annotations: string[] = []) {
-    super(...[value.getCoefficient(), value.getExponent(), value.isNegative()]);
+    super(value.numberValue());
     this._decimalValue = value;
     this._numberValue = value.numberValue();
     this._setAnnotations(annotations);
diff --git a/src/events/IonEvent.ts b/src/events/IonEvent.ts
index 217e53d..c78766c 100644
--- a/src/events/IonEvent.ts
+++ b/src/events/IonEvent.ts
@@ -468,7 +468,7 @@ class IonIntEvent extends AbstractIonEvent {
     }
     return new ComparisonResult(
       ComparisonResultType.NOT_EQUAL,
-      this.ionValue + " vs. " + expected.ionValue
+      `${this.ionValue} vs. ${expected.ionValue}`
     );
   }
 
@@ -870,7 +870,7 @@ class IonListEvent extends AbsIonContainerEvent {
       ) {
         return new ComparisonResult(
           ComparisonResultType.NOT_EQUAL,
-          child.ionValue + " vs. " + expectedContainer[i].ionValue,
+          `${child.ionValue} vs. ${expectedContainer[i].ionValue}`,
           i + 1,
           i + 1
         );


Test patch:
diff --git a/test/IonAnnotations.ts b/test/IonAnnotations.ts
index b19df28..185350d 100644
--- a/test/IonAnnotations.ts
+++ b/test/IonAnnotations.ts
@@ -15,6 +15,7 @@
 
 import {assert} from 'chai';
 import * as ion from '../src/Ion';
+import JSBI from "jsbi";
 
 function readerToBytes(reader) {
     let writer = ion.makeTextWriter();
@@ -33,7 +34,7 @@ describe('Annotations', () => {
         let reader = ion.makeReader(data);
         reader.next();
         assert.deepEqual(reader.annotations(), ['a', 'b']);
-        assert.equal(reader.value(), '123');
+        assert.equal(reader.value()?.toString(), '123');
         assert.equal(readerToString(reader), 'a::b::123');
     });
 
diff --git a/test/IonParserBinaryRaw.ts b/test/IonParserBinaryRaw.ts
index ec80f87..464b645 100644
--- a/test/IonParserBinaryRaw.ts
+++ b/test/IonParserBinaryRaw.ts
@@ -17,6 +17,7 @@ import {assert} from 'chai';
 import SignAndMagnitudeInt from "../src/SignAndMagnitudeInt";
 import {BinarySpan} from "../src/IonSpan";
 import {ParserBinaryRaw} from "../src/IonParserBinaryRaw";
+import JSBI from "jsbi";
 
 /**
  * Tests for reading the UInt primitive follow.
@@ -49,7 +50,8 @@ let unsignedIntBytesMatchValue = (bytes: number[],
                                       ParserBinaryRaw._readUnsignedIntAsBigIntFrom) => {
     let binarySpan = new BinarySpan(new Uint8Array(bytes));
     let actual = readFrom(binarySpan, bytes.length);
-    assert.equal(actual, expected)
+    actual = actual instanceof JSBI ? JSBI.toNumber(actual) : actual;
+    assert.equal(actual, expected);
 };
 
 let unsignedIntReadingTests = [
@@ -369,4 +371,4 @@ describe("Reading floats", () => {
             );
         });
     });
-});
\ No newline at end of file
+});
diff --git a/test/dom/Value.ts b/test/dom/Value.ts
index 608a815..52b7a40 100644
--- a/test/dom/Value.ts
+++ b/test/dom/Value.ts
@@ -109,10 +109,10 @@ function domValueTest(jsValue, expectedIonType) {
   return () => {
     // Test Value.from() with and without specifying annotations
     let annotations = ["foo", "bar", "baz"];
-    it("" + jsValue, () => {
+    it(`${jsValue}`, () => {
       validateDomValue(Value.from(jsValue), []);
     });
-    it(annotations.join("::") + "::" + jsValue, () => {
+    it(annotations.join("::") + `::${jsValue}`, () => {
       validateDomValue(Value.from(jsValue, annotations), annotations);
     });
   };
diff --git a/test/dom/dom.ts b/test/dom/dom.ts
index ec6cb76..c31b747 100644
--- a/test/dom/dom.ts
+++ b/test/dom/dom.ts
@@ -223,7 +223,7 @@ describe('DOM', () => {
       assert.equal(101.5, +d);
       assert.equal(101.5, d.numberValue());
       assert.isTrue(new ion.Decimal('101.5').equals(d.decimalValue()!));
-      assert.equal(1015, +d.decimalValue()!.getCoefficient());
+      assert.equal(1015, JSBI.toNumber(d.decimalValue()!.getCoefficient()));
    });
 
    it('load() Decimal as any', () => {
@@ -234,7 +234,7 @@ describe('DOM', () => {
       assert.equal(101.5, d);
       assert.equal(101.5, +d);
       assert.isTrue(new ion.Decimal('101.5').equals(d.decimalValue()));
-      assert.equal(1015, +d.decimalValue()!.getCoefficient());
+      assert.equal(1015, JSBI.toNumber(d.decimalValue()!.getCoefficient()));
    });
 
    it('load() Timestamp as Value', () => {
@@ -549,4 +549,4 @@ describe('DOM', () => {
       // length is 1 with two values: 55, 41
       assert.equal(1, s.fields().length)
    });
-});
\ No newline at end of file
+});
diff --git a/test/mochaSupport.ts b/test/mochaSupport.ts
index fe8a4ed..b727340 100644
--- a/test/mochaSupport.ts
+++ b/test/mochaSupport.ts
@@ -7,7 +7,7 @@ export function valueName(value: any): string {
     }
     if (typeof value === "object") {
         let typeText = _hasValue(value.constructor) ? value.constructor.name : 'Object';
-        let valueText = _hasValue(value.toString) ? ''+value : JSON.stringify(value);
+        let valueText = _hasValue(value.toString) ? `${value}` : JSON.stringify(value);
 
         return `${typeText}(${valueText})`;
     }