move to native BigInt from JSBI

[delaneyj]

Issue #, if available:
#477

Description of changes:

1. Removed all references to JSBI and replaced with bigint equivalent calls
2. Moved to a es2020 target as its necessary assumption for BigInt support
3. Ran prettier formatting on any effected file. Did not see a prettier config so assuming the defaults
4. Organize imports on effected files to ensure ordering and removal of unused imports.
5. 100% of tests pass locally

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

I confirm, currently BigInt->string->JSBI.BigInt is a big pain and slow

[desaikd]

Hello delaneyj@
Thanks for your contribution! The above change for moving to BigInt is something we have been looking to make ourselves for ion-js. Although this requires a new major version bump and is a breaking change, which is why we have some due diligence to before going ahead with the merge. Also, in addition to this change we have to communicate with our existing use base to determine stock of changes that needs to be added with the new major version release.

That aside, I did look at your PR. All the changes there are mostly converting JSBI reference to BigInt and seems good. Although the GitHub Action checks are failing, would you be able to look into that?

[delaneyj]

@desaikd I was sure it would be a breaking change and need a semver bump.

The 10.x fail appears to be on a line that has nothing to do with my change.

Khushboo, 23 months ago (September 29th, 2020 12:09 PM)

Maybe moving to es2020 (BigInt support) has issues in 10.x? I checked and 10.24.1 should have it. The other 2 errors are in a python app ion-test-driver.py which I don't quite understand. If there is docs you can point me to on how to run this stuff locally I can try to work out, but npm test run locally with no problems.

[popematt]

Since this requires a major version bump, we are happy to drop support for Node 10 and 12 (both of which are unsupported), and I'm moving your PR so that it's against the v5.0.0-development branch instead of master.

ion-test-driver.py comes from amzn/ion-test-driver, which is a cross-implementation test driver to help validate that ion-js, ion-java, etc. are consistent with one another. I haven't looked too deeply, but it looks like the failed workflow (from ~2 days ago) was using python 3.10, and we only updated ion-test-driver to work with python 3.10 today. I'll re-run the workflow and see if the recent change fixes it. Either way, we (the Ion maintainers) will take responsibility for figuring it out.

[delaneyj]

Sorry, having a hard time reading that output. It appears all the tests pass but it fails. I seem warnings, but no errors.

[desaikd]

The latest build failure might be because we didn't yet change the tsconfig.es6.json file with module pointing to es2020 and remove usage of es6.

[delaneyj]

The latest build failure might be because we didn't yet change the tsconfig.es6.json file with module pointing to es2020 and remove usage of es6.

Is this something I should do or is it an internal Amazon thing?

[desaikd]

The latest build failure might be because we didn't yet change the tsconfig.es6.json file with module pointing to es2020 and remove usage of es6.

Is this something I should do or is it an internal Amazon thing?

Oh yes, you can do it. Sorry, I just meant to say the tsconfig.es6.json might be the issue here.

[delaneyj]

The latest build failure might be because we didn't yet change the tsconfig.es6.json file with module pointing to es2020 and remove usage of es6.

Is this something I should do or is it an internal Amazon thing?

Oh yes, you can do it. Sorry, I just meant to say the tsconfig.es6.json might be the issue here.

But how do I test it locally?

[desaikd]

The latest build failure might be because we didn't yet change the tsconfig.es6.json file with module pointing to es2020 and remove usage of es6.

Is this something I should do or is it an internal Amazon thing?

Oh yes, you can do it. Sorry, I just meant to say the tsconfig.es6.json might be the issue here.

But how do I test it locally?

You can test it locally by running npm install that includes all the grunt release tasks including the build for modules.

[delaneyj]

@desaikd can you run it again, hopefully those changes make the difference.

[desaikd]

@desaikd can you run it again, hopefully those changes make the difference.

Thanks for adding the change! I just ran the workflow again.

[delaneyj]

Looks like node is fine but not grokking the issues with ion-test-driver from the logs

[cheqianh]

I've linked the test-driver issue to amazon-ion/ion-test-driver#24. Please feel free to ignore it for now.

[desaikd]

Looks good 🚀 !
We can ignore the ion-test-driver issue for this PR as it is unrelated to your changes.
Thanks @delaneyj for making all the changes. This change should add significant performance improvement as it removes JSBI dependency. We will add the change into our v5.0.0-development branch for our next major version release.