Open Dunqing opened 1 year ago
Nice work but more test cases are required. In the documentation you add the following example,
y = new BigNumber('45987349857634085409857349856430985')
y.toBigInt() // 45987349857634085409857349856430985n
but your implementation of toBigInt
actually returns a BigInt
with value 45987349857634086842492183761649664
because you are converting the BigNumber's value to a JS number using +
.
A correct implementation would be the following. Note that I use toFixed
because BigInt
does not handle exponential notation.
P.toBigInt = function () {
if (!this.isInteger()) throw Error(bignumberError + 'Not an integer: ' + valueOf(this));
return BigInt(this.toFixed());
};
or, if we want to allow BigInts to be formed from non-integers, and to return null
for NaN
and Infinity
P.toBigInt = function () {
return this.isFinite() ? BigInt(this.integerValue(1).toFixed()) : null;
};
where the 1
is BigNumber.ROUND_DOWN
, but it could be left out if we wanted the current global ROUNDING_MODE
setting to be used when rounding non-integers, or we could allow the option of passing a rounding mode as an argument.
The most efficient implementation would be the following. Like the BigInt
constructor, it always throws on non-integers. It is easy for a user to call integerValue
before toBigInt
if required anyway.
P.toBigInt = function () {
if (!this.isInteger()) throw Error(bignumberError + 'Not an integer: ' + valueOf(this));
var str = coeffToString(this.c);
for (var i = this.e + 1 - str.length; i > 0; i--) str += '0';
return BigInt(this.s < 0 ? '-' + str : str);
};
Okay, so this PR needs your implementation of toBigInt
to be replaced with the one directly above and more tests to be added.
Thanks for taking the time here. This is a great implementation. I will replace it later
I replaced and added some tests. Do we need more test cases? what tests need to add?
No, it's all good. I'm still not 100% on including this because up to now this library uses ECMAScript 3 (ES3) features only and I am not sure I want to break that convention. I'll leave this PR open for now. Thanks for your efforts.
I'm still not 100% on including this because up to now this library uses ECMAScript 3 (ES3) features only and I am not sure I want to break that convention.
We could determine if the current environment supports BigInt or else throw an error so that we can support the modern browser without affecting ES3
users. do you think that would work?
No, it's all good. I'm still not 100% on including this because up to now this library uses ECMAScript 3 (ES3) features only and I am not sure I want to break that convention. I'll leave this PR open for now. Thanks for your efforts.
I think it will be useful to use BigInt
across the entire library and release a major release that will drop ES3 backward compatibility and only support all versions of Node and browsers versions that are past bigint
100% support (IE and mobile non-Safari/Chrome excluded).
So I would say, whatever browsers from 2021 and Node LTS of 2021 supported could be a baseline of the code should be used for a new major release. I'm sure it will improve performance and reduce the amount of the code.
We have versions of the software and libraries for a reason I guess :) For people that are living under the rock and haven't updated their runtimes to support BigInt and other goodies, well, they'll to continue use older versions.
close: #352