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

SDK: Improve NPM packaging for @namada/sdk #1403

Open
jurevans opened this issue Dec 12, 2024 · 0 comments · May be fixed by #1408
Open

SDK: Improve NPM packaging for @namada/sdk #1403

jurevans opened this issue Dec 12, 2024 · 0 comments · May be fixed by #1408

Comments

@jurevans
Copy link
Collaborator

jurevans commented Dec 12, 2024

The current @namada/sdk package is a utter nightmare for users to import into their project. This is mainly due to a couple factors:

  1. We provide exports for both Web & NodeJS targets (NodeJS is required to run any unit tests, and the wasm dependencies must be invoked in a different manner, synchronously vs async) - this requires us to map package.json in a weird way
  2. The directory containing crypto.namada.wasm & shared.namada.wasm must be mapped in their build script as a static asset, so that init can find them. I will investigate how this can be improved

Also, there is a related bug where initInline is specified but not being included in the package. This is necessarily an issue for webpack, but we currently use this in Vite, so it should absolutely be provided in the published package! The difference between inline and just web init is that inline loads it into the built JS file, and initWeb async fetches it (I think...). Different bundlers have different requirements. initInline could also be used in Webpack.

One potential work-around I considered is packaging Web & NodeJS packages separately. This could look like this:

  • @namada/sdk
    • initWeb
    • initInline
  • @namada/sdk-node
    • init (initNode)

A developer building a web app would then likely specify @namada/sdk as a dependency, and @namada/sdk-node as a devDependency. I'm not sure I like that - maybe we can just bundle both versions of the wasm inside the same package with better documentation on when to use each? crypto.namada.wasm & shared.namada.wasm are 574K & 4.8M in size, respectively, so maybe this isn't an issue.

NOTE the dist folder produced by the release script results in wasm assets in the following locations:

  • dist/crypto.namada.wasm
  • dist/shared.namada.wasm
  • dist/node/crypto/src/crypto/crypto_bg.wasm
  • dist/node/shared/src/shared/shared_bg.wasm

This seems to indicate that we are already bundling the Node version separately, though I'm not convinced that it is correct. What is the difference between, e.g., shared_bg.wasm & shared.namada.wasm? shared_bg.wasm is generated on yarn wasm:build. shared.namada.wasm seems to only be created by the apps/extension webpack:

const copyPatterns = [
 {
    from: "../../packages/shared/src/shared/shared_bg.wasm",
    to: "./shared.namada.wasm",
  },
  {
    from: "../../packages/crypto/src/crypto/crypto_bg.wasm",
    to: "./crypto.namada.wasm",
  },
  ...
]

I'm not sure this should have to be required by the extension. I feel like if the paths are improved to always point where crypto_bg.wasm & shared_bg.wasm exist (for either web or node), then maybe this can be avoided? Users importing our SDK should not need to do something similar, like mapping to node_modules/@namada/sdk/shared.namada.wasm - right? We will eventually move the extension & Namadillo to their own repos, so this needs to be resolved by then.

Just for comparison, the wasm assets copied to dist on Namadillo appear like this (the SDK imports use the initInline initializer):

  • dist/assets/crypto_bg-{hash}.wasm
  • dist/assets/shared_bg-{hash}.wasm

Also, if possible, it would be much nicer if the SDK could be instantiated more simply. We pass in different initializers because of the different methods of loading the Wasm that need to be accounted for. Different init versions use different imports, so we can't simply abstract away the logic - these are each different entry points to account for that load the wasm in different ways.

NOTE One more thing to consider: When we run tests, we first issue the yarn wasm:build:node script versus yarn wasm:build. I suspect that the wasm asset in the published package (built with yarn wasm:build) is invalid for Node anyways, since we only provide one version. This would make a good case for providing a separate @namada/sdk-node package with the Node-targeted wasms, and Node-specific initializers. Otherwise, we would need to provide these in addition to the web targets!

Updated release build steps

(work in progress)

Crypto & Shared wasm builds for both targets:

  • Web
    • shared | crypto
      • yarn wasm:build
      • cp shared/src/shared dist/web/src/shared
      • cp crypto/src/crypto dist/web/src/crypto
  • Node
    • shared | crypto
    • yarn wasm:build:node
    • cp shared/src/shared dist/node/src/shared
    • cp crypto/src/crypto dist/node/src/crypto

Reference

Output of yarn wasm:build (wasm-pack) of crypto and shared libs:

In packages/crypto/src/:

crypto/
  ...
  crypto.d.ts
  crypto.js
  crypto_bg.wasm
  crypto_bg.wasm.d.ts
  package.json

In packages/shared/src:

shared/
  ...
  snippets/
    shared-801e3fcea73932a3/
      src/
        sdk/
          masp.node.js
        mod.js
      rpc_client.js
  package.json
  shared.d.ts
  shared.js
  shared_bg.wasm
  shared_bg.wasm.d.ts

It may be necessary that both crypto & shared exist under web & node packages, both containing their respective crypto_bg.wasm & shared_bg.wasm files. I don't think we need shared.namada.wasm or crypto.namada.wasm, but our SDK imports expect those to be defined, even though they won't be defined until webpack runs in apps/extension.

Would it be beneficial to move both crypto & shared into packages/sdk, and specifically point initWeb and initInline to a web build, and initNode to the node? Currently, we re-use the same output location when building wasm, but when releasing, we actually need both. Some of these issues are likely the result of how the project is currently organized, and that could certainly be improved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant