build: Use Uint8Arrays instead of Node Buffers (and update packages)

liftedinit / many-js

JavaScript implementation of the MANY protocol

Apache License 2.0

5 stars 7 forks source link

build: Use Uint8Arrays instead of Node Buffers (and update packages) #120

Closed stanleyjones closed 7 months ago

stanleyjones commented 11 months ago

Lots of good stuff in this one.

The primary motivation is to be able to upgrade downstream packages (e.g. Alberto, Gwen) to allow modern versions of Webpack and Node. Webpack 5+ no longer polyfills Node libraries by default, meaning we either have to polyfill it ourselves (which was giving errors in Many Address strings) or switch to use Uint8Arrays. More on that issue here: https://sindresorhus.com/blog/goodbye-nodejs-buffer

In order to pull this off correctly, this PR also beefs up the testing suite with lots of new tests and testing in both Node and JSDOM (i.e. browser) environments. Running npm test will run all the tests in both environments by default.

stanleyjones commented 11 months ago

Okay, CI is passing so I'll get to resolving conflicts now.

stanleyjones commented 11 months ago

And conflicts resolved too. Assigning for review now.

fmorency commented 11 months ago

Nice! Thanks a lot, @stanleyjones! I still need to review it, but I used it for another test project, and it worked perfectly for my purpose on Node 20.

fmorency commented 11 months ago

A lot of services/types are missing at the moment to be able to port Alberto/Gwen.

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (next@b3b8b82). Click here to learn what that means.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## next #120 +/- ## ======================================= Coverage ? 83.51% ======================================= Files ? 42 Lines ? 1207 Branches ? 58 ======================================= Hits ? 1008 Misses ? 199 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fmorency commented 11 months ago

I spent a couple of hours playing with this PR, and here are some of my comments.

I think the design is an improvement on the old version 👍🏻.
However, a lot is missing from this PR. This is a complete refactor of the library, not a "simple" workaround for the Buffer and package upgrade issues.
I don't particularly appreciate that some serialization/deserialization is done manually, i.e., mapToObj and objToMap. Why can't we use an existing library for this, e.g., cbor or cbor2? This should be transparent to the user.
There is no tagged Address type, i.e., tag 10000. Addresses are currently string. It should be its type that knows how to handle itself.

Finishing this PR and porting our software to the new version would require much effort. The risks are high, and I'm still debating if the effort is worth it versus migrating our chain and tools to a CosmosSDK solution.