GoogleChromeLabs / jsbi

JSBI is a pure-JavaScript implementation of the official ECMAScript BigInt proposal.
https://v8.dev/features/bigint#polyfilling-transpiling
Apache License 2.0
919 stars 68 forks source link

Throw an error when a JSBI instance is implicitly converted via valueof. #78

Closed 12wrigja closed 3 years ago

12wrigja commented 3 years ago

While reviewing https://github.com/js-temporal/temporal-polyfill/pull/77 reviewers noticed several places in code where JSBI instances were implicitly being converted to NaN, which seems undesirable.

12wrigja commented 3 years ago

Open question: this does seem like a breaking change, so it warrants a major version bump? Any issues with that?

jakobkummerow commented 3 years ago

I suppose it should get a major version bump, yes. (Which is a bit silly for something so minor; but it does turn a potential silent failure into an exception, so I agree that strictly speaking that's a breaking change.)

12wrigja commented 3 years ago

Turns out that converting a JSBI instance into a native BigInt by passing it as the input to the constructor "just works" (globalThis.BigInt(JSBI.BigInt("10")) === 10n is true). This is because the constructor will fall back to calling toString if valueOf is not defined. By adding in a valueOf function that throws this no longer works - users would have to explicitly call toString on JSBI instances before passing that into the native BigInt constructor.

Are we ok with this sort of breaking change?