jsbi
jsbi copied to clipboard
GoogleChromeLabs
Error: Convert JSBI instances to native numbers using `toNumber`
After updating to 4.1.0 release, getting the following error:
Stack trace:
"Error: Convert JSBI instances to native numbers using toNumber."," at JSBI.valueOf (/var/task/index.js:96308:1240)"," at new Number (
Code:
{ amount: { value: charge.get('cost', 'value')?.numberValue(), currency: charge.get('cost', 'currency')?.stringValue(), }, };
where charge is of type https://github.com/amzn/ion-js/blob/master/src/dom/Value.ts#L22
After a bit of digging, it looks like this patch, which was meant to make accidental uses of +JSBI.BigInt(42) throw an error, has a side effect of also making new Number(JSBI.BigInt(42)) throw, which is less desirable considering that native BigInts allow that (new Number(42n)).
IIUC, where things specifically go wrong is https://github.com/amzn/ion-js/blob/master/src/dom/Decimal.ts#L34, where the base class is Number (indirectly; it's wrapped in newClass), and value.getCoefficient() returns a JSBI instance, so the super call ends up doing new Number(some_jsbi). I'm not sure whether the fact that three arguments are being passed to the Number constructor is a bug; it doesn't seem useful to me as two of those will just be discarded -- maybe that code should do super(value.numberValue()) instead?
@12wrigja , what do you think? Should we just revert that patch, because silently permitting some bugs is better than refusing some valid patterns?
Sorry for the delayed response - I realize I had drafted a response but never sent it.
My personal opinion is that I would rather refuse valid patterns in favour of avoiding suble bugs (e.g. +JSBI.BigInt(42) + 42 is 84 without that patch, is a TypeError if directly transpiled to native BigInt +42n + 42, and is an Error with the patch mentioned above), only so long as there is an alternative way to express that pattern (explicitly calling Number(JSBI.toNumber(some_jsbi))).
That being said, JSBI.BigInt(42) + 42 returns 4242, so maybe that bogus return value is enough of a deterrent.
