-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds BigInt support to stringify util function. #4112
Conversation
I already signed the Contributor License Agreement, i don't know why does it show like pending, is there a way to reset it to sign it again? |
it could be a mismatch between username / email address. some ideas:
|
the test should be failing in IE11 and maybe safari... hmm. |
@boneskull is there something i am missing? |
@boneskull any follow up on this? |
just that those don’t support BigInt but I’m confused about why they weren’t failing CI. could you look in to it? I don’t have time atm. hoping to have more time for project after new year |
From what i see, the CI runs tests only on different NodeJs versions, Node 8 doesn't support BigInts either but I only added a new case to the stringify function, in that case it uses the toString function to create the representation of the BigInt, so the code it's never instantiating a BigInteger, just knows how to handle one if it ever receives it. |
Mocha just hangs for me when it tries to assert equality of two different bigints (it's fine when they are equal though). Might it be related to the issue that this PR fixes? @boneskull any chance this PR gets merged anytime soon? Sorry to nudge. |
👍 BigInt support is widespread in 2021: https://caniuse.com/?search=bigint |
I rebased to |
GH actions has problems with downloading an artifact and therefore the ESLint tests fail. It doesn't seem to be related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JosejeSinohui thank you. I apologize for the long delay.
this is a cherry-pick of 9122909
Description of the Change
Fixes #4090, adds a new case to the _stringify function in utils so it can handle the BigInt case.
Alternate Designs
There were two choices considered in the issue
BigInt<123>
or123n
i went with the latter because it's the standard way JS presents BigIntsWhy should this be in core?
Nice way of representing BigInts in errors.
Benefits
Support for Stringify-ing BigInts
Possible Drawbacks
I had to add
/* global BigInt */
at the top of the spec file, i don't know if that is the best place for it.Applicable issues
Fixes #4090