nucypher / nucypher-core

Core structures for Nucypher network in Rust
6 stars 11 forks source link

Inline WASM in `@nucypher/nucypher-core` NPM package #83

Closed piotr-roslaniec closed 1 year ago

piotr-roslaniec commented 1 year ago

Type of PR:

Required reviews:

What this does:

Issues fixed/closed:

Why it's needed:

Notes for reviewers:

codecov-commenter commented 1 year ago

Codecov Report

Merging #83 (f96a30f) into main (d49e98a) will not change coverage. Report is 2 commits behind head on main. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage   23.45%   23.45%           
=======================================
  Files          18       18           
  Lines        3521     3521           
=======================================
  Hits          826      826           
  Misses       2695     2695           
manumonti commented 1 year ago

The contents of ./nucypher-core-wasm-bundler could end up in the ./nucypher-core-wasm directory, however, I think the current packaging setup in ./nucypher-core-wasm is worth preserving. We could either document the two distinct approaches or merge them. TBD.

Some thoughts fwiw:

What could be the cost of keeping the two approaches? Does it involve duplicate work when updating the WASM packages?

If you ask me, my opinion is that is better to keep it simple with only one approach. But I’m not sure about the implications of deprecating the current approach for current adopters.

piotr-roslaniec commented 1 year ago

@manumonti

What could be the cost of keeping the two approaches? Does it involve duplicate work when updating the WASM packages?

We would only use the new approach when publishing an npm package. We could keep the "legacy" approach as a reference for someone wanting to build a non-inlined package.

derekpierre commented 1 year ago

This is a breaking change for @nucypher/nucypher-core users who relied on the previous packaging and WASM module instantiating methods.

How much work would it take someone you used the previous packaging process, to use this updated one? Is it relatively simple once the steps are known? New tooling? Should this process (migration from prior packaging) be documented alongside fresh (only this new) usage?

piotr-roslaniec commented 1 year ago

How much work would it take someone you used the previous packaging process, to use this updated one? Is it relatively simple once the steps are known? New tooling?

It only requires calling await initialize() instead of using a bundler.

Should this process (migration from prior packaging) be documented alongside fresh (only this new) usage?

We can document it in a changelog. @nucypher/nucypher-core is an intermediate dependency with no known external users so I don't think this is going to be an issue.

piotr-roslaniec commented 1 year ago

I found an issue caused by the changes in this PR: test runners (like jest) have issues calling the initialize method. I will address it in this PR.

piotr-roslaniec commented 1 year ago

Because of the inlining the size of @nucypher/nucypher-core increased by roughly ~33%.

image https://bundlephobia.com/scan-results?packages=@nucypher/nucypher-core@0.13.0-alpha.1

jMyles commented 1 year ago

We can document it in a changelog. @nucypher/nucypher-core is an intermediate dependency with no known external users so I don't think this is going to be an issue.

Moreover, it makes sense for this to be our public stance in general with regard to this repo (and perhaps to finally examine whether we want to call it 'core' rather than something like 'nucypher-base').