-
Notifications
You must be signed in to change notification settings - Fork 433
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
Edit/create rollup configs to add separate node/browser entrypoints #339
base: master
Are you sure you want to change the base?
Conversation
@phated would appreciate y'all take a look - regardless of whether it's this specific set of changes or something else, I think it's important to ship a separate importable bundle for browser/non-node environments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! I have some feedback about changing the paths. Let's get those resolved and we can plan to merge this.
Tangent: I need to join all the build commands together because the new ones added will get missed when Jordi cuts releases.
@@ -19,14 +19,17 @@ export const O_RDONLY = ${O_RDONLY} | |||
export default { | |||
input: "main.js", | |||
output: { | |||
file: "build/snarkjs.js", | |||
file: "build/snarkjs.iife.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file: "build/snarkjs.iife.js", | |
file: "build/snarkjs.js", |
let's keep this since it is breaking if people are reaching into this path
...config, | ||
output: { | ||
...config.output, | ||
file: "build/snarkjs.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file: "build/snarkjs.js", | |
file: "build/snarkjs.esm.js", |
We can make this .esm.js
so the iife path doesn't change.
"import": "./main.js", | ||
"require": "./build/main.cjs" | ||
}, | ||
"default": "./build/snarkjs.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"default": "./build/snarkjs.js" | |
"default": "./build/snarkjs.esm.js" |
}, | ||
"scripts": { | ||
"test": "mocha", | ||
"build": "rollup -c config/rollup.cjs.config.js", | ||
"buildcli": "rollup -c config/rollup.cli.config.js", | ||
"buildesm": "rollup -c config/rollup.esm.config.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"buildesm": "rollup -c config/rollup.esm.config.js", |
This one doesn't exist
treeshake: { | ||
moduleSideEffects: false | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
Note that this didn't solve #317 but instead worked around some of the stuff, so I'm going to remove that from the root comment. |
Makes sense! |
This PR adds and updates the
rollup.config
files to build aESM
bundle for bothnode
andbrowser
environments, and then reference them in thepackage.json
. This should greatly increase support for bundlers/build systems such as swc (NextJS), vite, etc, etc, etc. As long as thenode
condition/flag is not present during bundling, bundlers should default tosnark.js
which has been converted to a browser targeted ESM file. It should improve the dev-x to help people avoid people running into the issues seen in #97 and #113.TODO:
Test with Parcel(Parcel has incredibly bad static asset support which makes it near impossible to bundle zkeys)Rationale
Currently, importing
snarkjs
in a browser project, such as a NextJS/SWC or React/Vite project, often leads to a lot of build/runtime errors. This is because thepackage.json
defines conditional exports like this:Many developers are working with build systems which use ESM
import
syntax, meaning that the package resolves tomain.js
. This would not be a problem, except that because there are so many build system stacks (tsup, parcel, rollup, swc, esbuild, webpack, etc etc etc) in which conditions likeprocess.browser
referenced in the snarkjs code are not set properly. In cases where it is not set properly (like #317), this leads to code depending on node-exclusive interfaces making it through code-elimination and getting bundled into browser frontends without the proper polyfills.Even though snarkjs ships with a IIFE bundle, as can be seen in
rollup.iife.config.js
, it's not explicitly exported in package.json, nor is its existence well documented. As a result many new applied practitioners run into esoteric bundling errors - as can be seen in #113 and #97. In cases where the IIFE bundle's existence is known to the developers, they often choose to hardcode the file into their frontend, circumventing modern bundling processes entirely - for example, see this example in heyanoun. This is not ideal, as it removes snarkjs from package management, meaning that people have to manually update the minified artifact. This is another point of failure in systems where points of failure can violate privacy.Another possible solution for people running into this problem is to import directly from "node_modules". However this means their imports become more brittle as they must be relative. For example.
I find it unsatisfactory that the introductory experience many people have with deploying snarkjs on the web involves them dipping into a web bundler/static assets bundling rabbithole that many don't escape from. In the most egregious case, people get around this by spinning up a small backend specifically for generating proofs, which loosens their project's trust assumptions. Wouldn't it be nice if people could just write
regardless of environment? Even in environments where this works, it often results in a bunch of unnecessary code getting bundled as well.
While there are a few possible solutions to achieve this, I propose updating the rollup configs and changing the
exports
field inpackage.json
to the following:Another possible solution is to add a conditional export for the "browser" environment. However, this extends direct import support to only environments that explicitly define the browser flag, whereas any environment bundler that is not a "node" environment should get some heads up that it cannot access node interfaces. This is the approach recommended here: https://nodejs.org/api/packages.html#conditional-exports.
The "node" condition was introduced in later versions of Node 12, so since snarkjs recommends Node 16, I reckon this won't break much.
As a stretch goal it may be nice to add some build e2e integration tests similar to the ones seen here: https://github.com/Mokshit06/typewind/tree/main/tests, which would build a package for an environment (node/browser/minified JS) and generate and verify a simple proof within a frontend, though this may be a separate PR.
Looking for feedback on
node
orbrowser
export condition