near / borsh-js

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

Add ESM support #44

Closed volovyks closed 1 year ago

volovyks commented 2 years ago

Let's add ESM alongside CJS to this project. Can be a requirement for https://github.com/near/near-api-js/issues/607 cc @itegulov

cristobal commented 2 years ago

Let's add ESM alongside CJS to this project. Can be a requirement for near/near-api-js#607 cc @itegulov

Why not remove dependency on text-encoding-utf-8 and make it an optional polyfill, since the module actually does not export an default which is the one used here

// TODO: Make sure this polyfill not included when not required
import * as encoding from "text-encoding-utf-8";

Leaving this part out or modifying the text-encoding-utf-8 to include a default export, works when building with Esbuild or Snowpack i commented this in #47.

cristobal commented 2 years ago

Perhaps some contribution to both the bn.js and bs58 to have proper esm build and exports?

Not sure if bs58 should be an dependency, due to the few lines of code:

const basex = require('base-x')
const ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz'

module.exports = basex(ALPHABET)

perhaps rather just depend on base-x instead?

volovyks commented 2 years ago

@cristobal looks reasonable! Do you think you can create a PR?

cristobal commented 2 years ago

@cristobal looks reasonable! Do you think you can create a PR?

Sure could try do make some PR's, what do you prefer one big PR or several smaller changes?

@volovyk-s Thoughts on the process?

itegulov commented 2 years ago

@cristobal if you do start working on this issue feel free to take a look at my attempt I made a couple of weeks ago: https://github.com/near/borsh-js/commit/bf616b51e170ea8b7c64444cf1628cf907e02b3d. As you said, I had to get rid of bs58, but base-x itself did not have ESM support either so I tried to temporarily copy its content in place. The end result seemed to work fine with vite, so I am not sure if we actually need to do anything about bn.js and text-encoding-utf-8, but I could very well be wrong.

volovyks commented 2 years ago

Thanks, @cristobal ! Let's move step by step. We can merge breaking changes to a separate branch and then release them together with a new major version update.

cristobal commented 2 years ago

Ok will start working on this next week, if i manage to get some time free this weekend i will look into it but otherwise expect some PR's from next week 🙂 👍🏽

volovyks commented 2 years ago

@cristobal I have created develop-breaking branch for breaking changes. You can target it in your PR's.

ailisp commented 1 year ago

Supported in 1.0!