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

Edit/create rollup configs to add separate node/browser entrypoints #339

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sigmachirality
Copy link

@sigmachirality sigmachirality commented Mar 21, 2023

This PR adds and updates the rollup.config files to build a ESM bundle for both node and browser environments, and then reference them in the package.json. This should greatly increase support for bundlers/build systems such as swc (NextJS), vite, etc, etc, etc. As long as the node condition/flag is not present during bundling, bundlers should default to snark.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 bundling with React/Vite
  • Test with Parcel (Parcel has incredibly bad static asset support which makes it near impossible to bundle zkeys)
  • Test with Nextjs (swc)
  • Test with Rollup
  • Test minified module still works

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 the package.json defines conditional exports like this:

"exports": {
   "import": "./main.js",
   "require": "./build/main.cjs"

Many developers are working with build systems which use ESM import syntax, meaning that the package resolves to main.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 like process.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.

import { groth16} from "../../../../../node_modules/build/snarkjs.js"

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

import { groth16 } from "snarkjs"

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 in package.json to the following:

"exports":
   "node": {
       "import": "./main.js",
       "require": "./build/main.cjs"
    },
    "default": "./build/snarkjs.js"

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.

When using environment branches, always include a "default" condition where possible. Providing a "default" condition ensures that any unknown JS environments are able to use this universal implementation, which helps avoid these JS environments from having to pretend to be existing environments in order to support packages with conditional exports. For this reason, using "node" and "default" condition branches is usually preferable to using "node" and "browser" condition branches.

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

  • naming rollup config files (should the new file be snarkjs.js, or should the old IIFE be?)
  • whether to use the node or browser export condition
  • whether this makes sense? (I think it does). For example, it may be extraneous to replace the current "import" exports entrypoint as it seems to work fine.

@sigmachirality
Copy link
Author

sigmachirality commented Mar 21, 2023

@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.

@sigmachirality sigmachirality changed the title Edit/create rollup configs to add ESM/browser entrypoints Edit/create rollup configs to add separate node/browser entrypoints Mar 21, 2023
Copy link
Contributor

@phated phated left a 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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"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",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"buildesm": "rollup -c config/rollup.esm.config.js",

This one doesn't exist

Comment on lines +30 to +32
treeshake: {
moduleSideEffects: false
},
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

@phated
Copy link
Contributor

phated commented May 1, 2023

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.

@sigmachirality
Copy link
Author

Makes sense!

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

Successfully merging this pull request may close these issues.

2 participants