Skip to content
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

Add support for BigInt #87

Merged
merged 9 commits into from
May 4, 2019
Merged

Add support for BigInt #87

merged 9 commits into from
May 4, 2019

Conversation

vladfrangu
Copy link
Contributor

@vladfrangu vladfrangu commented May 2, 2019

Fixes #39

  • Implements support for the BigInt, BigInt64Array and BigUint64Array

  • All tests pass

@sindresorhus
Copy link
Owner

Why are you removing Node.js 8 support? That seems unrelated.

@vladfrangu
Copy link
Contributor Author

I thought nodejs 8 didn't have bigint support. Let me check real quick...

@vladfrangu
Copy link
Contributor Author

vladfrangu commented May 2, 2019

image

nodejs 8 does NOT have BigInt support. (quite a shame too)

EDIT: There probably is bigint support in nodejs 8 through a harmony flag, but that's still far more than the average person will do...

@vladfrangu vladfrangu changed the title src: Implement BigInt support, remove nodejs 8 src: Implement BigInt support May 2, 2019
@sindresorhus
Copy link
Owner

We're not actually using BigInt, we're just detecting it, so we can still preserve support for Node.js 8. You can just comment out the tests for now.

tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title src: Implement BigInt support Add support for BigInt May 3, 2019
@vladfrangu
Copy link
Contributor Author

Yep, after reading your review, I realized you only did typeof / toString calls and checks, which would mean nodejs 8 support would still be present! I've done your requested changes now, as well as commented out all tests that only work on nodejs >=10. Hope this helps!

.travis.yml Outdated Show resolved Hide resolved
@vladfrangu
Copy link
Contributor Author

@sindresorhus should be done now! If there's any other changes you'd like me to do, please let me know 👍

@sindresorhus sindresorhus merged commit dd2a91d into sindresorhus:master May 4, 2019
@sindresorhus
Copy link
Owner

Nice work 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support BigInt detection
2 participants