Allow for Browser Compatibility

ripple / ripple-binary-codec

Convert between json and hex representations of transactions and ledger entries on the XRP Ledger. Moved to: https://github.com/XRPLF/xrpl.js/tree/develop/packages/ripple-binary-codec

https://github.com/XRPLF/xrpl.js/tree/develop/packages/ripple-binary-codec

ISC License

19 stars 45 forks source link

Allow for Browser Compatibility #107

Closed natenichols closed 3 years ago

natenichols commented 3 years ago

High Level Overview of Change

This change makes it possible to run this in the browser

Context of Change

With the packages we were using, nodeJS buffers would be compiled down to browser compatible buffers. These browser compatible buffers do not have the API for readBigUInt64BE and writeBigUInt64BE. This rewrite makes the binary-codec compatible with these tools.

Type of Change

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[x] Refactor (non-breaking change that only restructures code)
[ ] Tests (You added tests for code that already exists, or your new feature included in this PR)
[ ] Documentation Updates
[ ] Release

Test Plan

No functionality was changed, all tests still pass.

natenichols commented 3 years ago

That would be correct. It will not work in Browsers w/o support for BigInt. How important is it that old browsers support ripple-lib?

Since new browsers support it, I don't see a problem using BigInt. If we wanna make sure it is compatible with old Browsers we can use a library to replace BigInt.

natenichols commented 3 years ago

LGTM. I wonder if we should add a note in the README about browser compatibility? Since we use BigInt now, will it work in browsers that don't support BigInt?

All new browsers support it, but old ones do not, like Internet Explorer and Safari versions prior to Safari 14.

I added a short note about browser compatibility. I'm also debating whether to continue to update v2.6 in order to support older browsers. All we would need to do to keep older versions up to date with the protocol is to update to the most recent definitions.json. Thoughts?

intelliot commented 3 years ago

I'm happy either way. I think the important thing is that we call out what the expected compatibility is, so that users are aware of which browsers are expected to work. If you'd like to update the 0.2.x branch, I think that is reasonable, too (especially since it's not much work). On the other hand, it's also ok to wait until someone complains :) Maybe no one will complain.

natenichols commented 3 years ago

I'm happy either way. I think the important thing is that we call out what the expected compatibility is, so that users are aware of which browsers are expected to work. If you'd like to update the 0.2.x branch, I think that is reasonable, too (especially since it's not much work). On the other hand, it's also ok to wait until someone complains :) Maybe no one will complain.

Sounds good to me. I'll merge and roll this into a beta release

Stormtv commented 3 years ago

@natenichols

BigInt only has 80% support atm I would recommend using https://www.npmjs.com/package/big-integer which acts as a polyfill for the 20% of users who don't have BigInt support.