search

near / borsh-js

TypeScript/JavaScript implementation of Binary Object Representation Serializer for Hashing
Apache License 2.0
112 stars 38 forks source link

Major Update of Borsh #65

Closed gagdiez closed 1 year ago

gagdiez commented 1 year ago

This is a complete rewrite of the library which includes multiple advantages over the current v0.7.0:

  1. Vanilla JS implementation, does not rely on Buffer, for which it is compatible with a greater number of environments.
  2. It can (de)serialize single values, objects and instances of Classes. Previously we were only able to (de)serialize specific instances of classes.
  3. The new Schema is consistent across types, and a thoughtful validation of the Schema is in place.
  4. Each serialize/deserialize relies on a single Schema, solving some longstanding problems with the previous "Indexing by Class" approach.
  5. Thoughtfully tested across multiple scenarios.

Since this is a full rewrite, I would recommend to directly look at the origin repository

ailisp commented 1 year ago

@gagdiez Could you also config this to build both commonjs and esm? Because near-sdk-js can only take esm modules. Here is a configuration that probably works: https://thesametech.com/how-to-build-typescript-project/

gagdiez commented 1 year ago

@ailisp build now compiles both cjs and esm versions, tests are still passing and integration with near-api-js works wells. Is there any simple way to test the esm version?

ailisp commented 1 year ago

Hi @gagdiez Thanks for your update. For esm I'd suggest to add an minimal test to ensure the packaging works. Because we're testing esm (and maybe also cjs and ts) packaging, not the functionality correctness, which you already covered in unit test.

This minimal test may look like this: have a dir and a package.json that declares as an ESM module and depends on packaged borsh-js. Test it can correctly import the API.

gagdiez commented 1 year ago

@ailisp I fixed the errors that did not allow to import it as a esm package, and included two examples showing both cjs and esm work.

BN (from bn.js) and bs58 presented no problems locally on the esm example, particularly since they have vanilla js implementations. Let me know if they do end up hampering the integration on near-sdk-js and will see to get them removed.

ailisp commented 1 year ago

Thanks @gagdiez ! We'll test again in near-sdk-js side

ailisp commented 1 year ago

Hi @gagdiez ! We tested your latest commit in near-sdk-js, now it can import borsh, but cannot import bn. I guess it will also not able to import bs58 but it only gives one error message at a time: https://github.com/fospring/near-sdk-js/pull/1#issuecomment-1660536213

I think why it works in your test (run with nodejs) is because nodejs support both commonjs and esm, so an esm dependency (borsh) depends on a commonjs (bn) is still working. However, near-sdk-js's setup only support pure esm modules: means all dependency and the dependency of dependency has to be esm.

I look at your code, I think we're not using some advanced feature of bn, so replacing it with native JS int could be fine?

gagdiez commented 1 year ago

@ailisp removed BN & bs58, it is now officially compatible with near-sdk-js (I see that @fospring merged the PR passing tests)