Add browser support, remove dependencies

[paulmillr]

Hi,

This probably won't get merged right now -- but I want to get the idea exposed.

Basically Buffer is not supported in browsers. An Uint8Array should be used instead, since all Buffers are actually Uint8Arrays. This would allow to support browsers out-of-box.

As for bn.js, all major browsers and node.js versions right now support native bigints. At some point rlp should switch to native bigints -- which would allow to drop dependencies. We could ditch bn.js change for now, but I think it's important to switch to Uint8Arrays at some point. The lib could be used in browsers a lot.

I've made a fork - all tests pass. Let me know what you think

[coveralls]

Coverage decreased (-0.6%) to 95.122% when pulling c222a4c on paulmillr:master into bca3d7d on ethereumjs:master.

[holgerd77]

Hi @paulmillr, thanks for bringing this up! This will definitely need some discussion, but also something worth to think about. On the BigInt changes, I personally think it is still a bit too early to draw these in since Safari has just only recently added support afaik, not even sure if only in the dev build or the main release. We should definitely handle these things (UInt8Array, BigInt) separately on discussion.

[paulmillr]

Great. Thanks for not closing this immediately!

[kumavis]

Node.js 10.4.0 has BigInt support.

TIL!

current browser support not great: Global 83.45% https://caniuse.com/bigint

[kumavis]

@paulmillr you should convert this to a "draft PR" if possible since its more of a proof of concept than a PR (e.g. renaming package)

[paulmillr]

@kumavis i've moved package renaming changes into a separate branch, so the current one only contains necessary changes.

[paulmillr]

Global browser support is 88.72% now

[paulmillr]

The "format" step errors are completely NOT helpful. Cannot tell what's wrong here. All unit tests pass.

[paulmillr]

global browser support is 91.22% now

[talentlessguy]

@paulmillr there seems to be a merge conflict rn

[paulmillr]

@talentlessguy yeah, I have no incentive to keep it up to date if it isn't planning to be merged.

[talentlessguy]

I see..

[ryanio]

hey @paulmillr thanks for being so early with this PR :)

we added BigInt support with #85 and browser support in #102. We output es5 and if people need a bundle they can typically do it with browserify or webpack, and can polyfill Buffer.

We have previously had some team discussions regarding switching from Buffers to Uint8Array for both performance and fewer dependencies. I think we would be open to it, and starting with a relatively small repository like this should be a good experiment.

Would you be able to update this PR to just include the Uint8Array change? I am also happy to take it over if you're busy with other things at the moment. I would be interested in doing a quick benchmark to see if it does make a noticeable difference in speed.

The only thing I am thinking now is that we have a PR in progress to dedupe some internal helper functions by adding ethereumjs-util (#104), but that has Buffer dependency.

[paulmillr]

Hi @ryanio

I work on supply chain security for npm and eth ecosystems.

1. The Remove BN dependency, add ethereumjs-util dependency #104 that you've mentioned removed 25? lines of code and replaced them with 300KB dependency that has at least 5 sub-dependencies
2. One of ethereumjs-util dependencies is rlp itself, so it's recursive? Seems bad.
3. Another dependency is ethereum-cryptography which i've been rewriting for a couple months to bring its number of dependencies from 39 to 3. I've also been coordinating an audit of it which should be finished in a couple days.
4. As ethereum-cryptography comes close to its first stable audited release, it will be using native bigints - without shims. It will use Uint8Arrays instead of buffers.
5. I duplicate 30LOC helpers through all noble cryptography I make because it does not make sense for supply chain security - for the libraries to depend on anything because of 30LOC.

So, you should definitely switch to bigints. If you won't, your dependencies will, in any case -- and very soon. I don't see how it makes sense for RLP to depend on anything. It should be a dead-simple 300LOC lib.

I can re-submit this PR that would bring dep count to 0. I don't see any point of RLP depending on bn.js. Bn.js is shitty, for real.

[ryanio]

@paulmillr thanks for the reply! that's helpful insight to avoid for ~30 LOC as you say.

yes I have been following some of your work on the ethereum-cryptography rework and it's really amazing. unfortunately we can't upgrade to native bigint support without breaking non-bigint environments completely, so we have to be careful about our upgrade paths and have to wait for major versions, but I think we are approaching it soon.

We also saw these benchmarks where bn.js was faster in some cases than BigInt, so that leaves us confused whether a total switch within our repositories would be worth it at all.

[paulmillr]

@ryanio If bn.js was faster than bigints, then my libraries would be slower than all those "established" ones which use all sorts of bn.js optimizations. They aren't, noble is many times faster. You need to see a real-env usage, not synthetic benchmarks that don't count garbage collection / ram impact on perf etc.

unfortunately we can't upgrade to native bigint support without breaking non-bigint environments completely

You will be upgraded automatically at some point in the future, just like all "core" ETH projects, since ethereum-cryptography is included everywhere.

Why wait? You can release 3.0 that's breaking "as per semver", include uint8array and native bigints and be done with it. Everyone who wants to support old environments can keep using 2.x, and we can provide updates to 2.x for a bit.

[paulmillr]

P.S. i've ran the benchmarks and commented in the gist you've mentioned - even in this synthetic benchmark native bigints are faster

[ryanio]

@paulmillr thanks for your continued feedback!

Do you think it would be better to go straight to BigInt, or use a polyfill-able library like JSBI that in the future could be transformed with e.g. babel-plugin-transform-jsbi-to-bigint?

If we consume ethereum-cryptography v2 maybe the JSBI step would be redundant since we'll already be expected to support native syntax?

[paulmillr]

Do you think it would be better to go straight to BigInt, or use a polyfill-able library like JSBI that in the future could be transformed with e.g. babel-plugin-transform-jsbi-to-bigint?

I haven't tested polyfills and have no feedback in my repos about polyfills during the last 2+ years. So, not really sure whether they work at all.

I think it doesn't make huge sense to use polyfills, because bigints are natively supported in all major environments. The only unsupported old ones are Safari+iOS Safari 13 (that's 2% of all users, the browsers are 2+ years old); Chrome <66 (0.64%, 3.5 years old); Firefox <67 (0.54%, 2.5 years old), IE11 (2013). Node supported bigints since 2018, all versions that ship security updates (LTS) support bigints.

2022 seems like a good time to use bigints in all sorts of apps.

[ryanio]

@paulmillr Alright great well I'd love to get this branch rebased on the latest master. I looked through the code and it all looks excellent. Let me know if that's okay with you, I can push to your remote or copy it over to our remote and work on it from there.

One thing I'd like to apply in the src is a trick we learned from #100 to avoid creating an extra slice when checking hex prefix: v.slice(0, 2) === '0x' -> v[0] === '0' && v[1] === 'x', which can be used as isHexPrefixed.

[paulmillr]

@ryanio I can rebase it, are you planning on merging it if everything goes fine?

[ryanio]

I have not yet asked the team to see if there is any hesitation against it, but I am in favor and would be motivated to do a rlp v3 release with zero deps, BigInt, and Uint8Arrays in place of Buffers, that seems nice to me. I will propose in our internal chat.

[paulmillr]

Cool, just don't want to spend another bunch of hours on something non-mergeable

[paulmillr]

Okay, this took three more hours, really hoping on getting this in @ryanio @holgerd77

Also, my test command fails all the time, not sure what's up, README doesn't specify build steps besides npm install. test:unit runs fine

> rlp@2.2.7 test
> npm run lint && npm run build && npm run test:unit && npm run test:browser


> rlp@2.2.7 lint
> ethereumjs-config-lint

> eslint --format codeframe --config ./.eslintrc.js . \ 
	 --ext .js,.jsx,.ts,.tsx \ 
rlp/node_modules/.bin/ethereumjs-config-lint: line 26: realpath: command not found
usage: dirname string [...]
usage: dirname string [...]
	 --resolve-plugins-relative-to 
[Lint] checking...
rlp/node_modules/.bin/ethereumjs-config-lint: line 26: realpath: command not found
usage: dirname string [...]
usage: dirname string [...]
Value for 'resolve-plugins-relative-to' of type 'path::String' required.

[paulmillr]

I'm also guessing a typescript compilation update would be needed because we're building file for browsers now. Not sure how to do it.

[ryanio]

@paulmillr thanks for updating and working on this! So far in response we have had all positive reception to the v3 idea so we can very likely make this happen :)

We might move this repository to the monorepo, or continue to develop on it standalone, undecided yet but will let you know, regardless we can still use this PR.

Regarding your realpath error, seems to be from our lint convenience package, try one of these options I found on StackOverflow?

Here are the steps to install realpath if it's not present:
Ubuntu: sudo apt-get install coreutils
OS X: brew install coreutils

---

I'm also guessing a typescript compilation update would be needed because we're building file for browsers now. Not sure how to do it.

Our build step on current master will output a dist.browser (built from tsconfig.browser.json), however we probably need to update this beyond the currently specified ES5 if we are to include BigInt support?

[ryanio]

Also, another library improvement suggestion, what do you think of default exporting an RLP class with static methods encode/decode, so people can do import RLP from 'rlp' and RLP.encode()/decode() rather than import * as RLP from 'rlp'?

Then if one wanted something extra like an exported type, could do import RLP, { Decoded } from 'rlp'

My reasoning is to prefer not to encourage use of namespace (wildcard *) imports, like https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-namespace.md

[ryanio]

Use single-line comments because typescript leaves them behind (97e391e)

What do you mean by this commit/comment? I found the multi-line syntax is helpful for viewing the tooltips in VS Code, where single line comments using // don't get picked up.

[ryanio]

For dev please run npm run lint:fix, sorry about the missing docs

[paulmillr]

unsure what's up with GitHub tests, they're running fine on my mac

[ryanio]

the tests look like they are passing now, what it's catching on now is a known karma-typescript issue which was fixed by monounity/karma-typescript#502 but there hasn't been a release yet.

you can try updating the package.json dependency to "karma-typescript": "monounity/karma-typescript" to see if that fixes.

[ryanio]

LGTM! thanks @paulmillr awesome stuff.

I will follow up with a PR to fix the karma runner which is where the ci is failing.

[HDauven]

This is an awesome addition, thanks @paulmillr!

I've had a use case where a browser version of RLP was required. A potential approach with regard to making Buffer usable in browser environments is by using this drop in replacement: https://github.com/feross/buffer
Advantage being that consumers of the RLP library do not have to think about polyfills.

In node environments it will just use the Node libraries, in browser environments it will use the imported library.

[holgerd77]

I've had a use case where a browser version of RLP was required. A potential approach with regard to making Buffer usable in browser environments is by using this drop in replacement: https://github.com/feross/buffer
Advantage being that consumers of the RLP library do not have to think about polyfills.

In node environments it will just use the Node libraries, in browser environments it will use the imported library.

@HDauven just to clarify: the comment from above is just referring to the existing versions of the RLP library, correct?

With an RLP v3 release using Uint8Array you should not have any problems with polyfilling anything respectively think about polyfills? 🤔

Or let me know if I misunderstood your comment in some way.