Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major Update of Borsh #65

Merged
merged 35 commits into from
Aug 4, 2023
Merged

Major Update of Borsh #65

merged 35 commits into from
Aug 4, 2023

Conversation

gagdiez
Copy link
Contributor

@gagdiez gagdiez commented Jul 21, 2023

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

@gagdiez gagdiez requested a review from ailisp as a code owner July 21, 2023 14:19
@gagdiez gagdiez requested a review from volovyks July 21, 2023 14:20
@ailisp
Copy link
Member

ailisp commented Jul 31, 2023

@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
Copy link
Contributor Author

gagdiez commented Jul 31, 2023

@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?

package.json Outdated Show resolved Hide resolved
@ailisp
Copy link
Member

ailisp commented Aug 1, 2023

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.

lib/esm/index.js Outdated Show resolved Hide resolved
lib/esm/serialize.js Outdated Show resolved Hide resolved
@gagdiez
Copy link
Contributor Author

gagdiez commented Aug 1, 2023

@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
Copy link
Member

ailisp commented Aug 1, 2023

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

@ailisp
Copy link
Member

ailisp commented Aug 2, 2023

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: fospring/near-sdk-js#1 (comment)

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
Copy link
Contributor Author

gagdiez commented Aug 2, 2023

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

package.json Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gagdiez! Thank you so much for this great job! Besides the other benefits you mentioned, We're able to run borsh in near-sdk-js and I think the new API is much cleaner, easier to use than 0.7. There is some minor things to address, and then ready to merge & release!

borsh-ts/serialize.ts Outdated Show resolved Hide resolved
@ailisp ailisp merged commit 93a633d into near:master Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants