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

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

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

piotr-roslaniec
Copy link
Contributor

@piotr-roslaniec piotr-roslaniec commented Sep 21, 2023

Type of PR:

  • Feature
  • Other

Required reviews:

  • 2

What this does:

  • Add a rollup based NPM package bundler in the ./nucypher-core-wasm-bundler directory
  • Configures WASM inlining for the @nucypher/nucypher-core package. Now, said package can be used without a WASM-aware bundler.

Issues fixed/closed:

Why it's needed:

  • Simplifies consuming @nucypher/nucypher-core

Notes for reviewers:

  • This is a breaking change for @nucypher/nucypher-core users who relied on the previous packaging and WASM module instantiating methods.
  • 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.
  • There is a trade-off when using this inlining technique - The base64-encoded WASM is about 34% larger than in the binary form. We can address it in the future by exposing a "slim" build variant of this library using package.json exports. Similarly, we could offer a node-friendly "slim" build. See rollup.config.js for details. However, these two changes require further research that I can't afford now. I opted to ship these changes earlier at a cost of less flexibility for our users.

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2023

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           

Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

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

LGTM! 🙌

I think this is a great progress to have a more user-friendly environment to be used by adopters.

@manumonti
Copy link
Member

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

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

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

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 piotr-roslaniec added the do not merge Reviews are welcome, but please do not merge label Sep 26, 2023
@piotr-roslaniec
Copy link
Contributor Author

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

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

jMyles commented Oct 3, 2023

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').

Copy link
Member

@cygnusv cygnusv left a comment

Choose a reason for hiding this comment

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

🚀

nucypher-core-wasm-bundler/rollup.config.js Outdated Show resolved Hide resolved
Co-authored-by: David Núñez <david@nucypher.com>
@piotr-roslaniec piotr-roslaniec merged commit f1be594 into nucypher:main Oct 5, 2023
11 checks passed
@piotr-roslaniec piotr-roslaniec deleted the inline-wasm branch October 5, 2023 09:08
@derekpierre derekpierre mentioned this pull request Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Reviews are welcome, but please do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants