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

Add Browser ES module build #127

Merged
merged 3 commits into from
Oct 26, 2023
Merged

Add Browser ES module build #127

merged 3 commits into from
Oct 26, 2023

Conversation

Kolezhniuk
Copy link
Contributor

@Kolezhniuk Kolezhniuk commented Oct 6, 2023

Add Browser ES module build

Why?

  • Out team is using snarkjs package in our project https://github.com/0xPolygonID/js-sdk and faced with issue that ES build for browser is not working. It works fine with snarkjs bundle for browser, but there are some disadvantages:
    1. it's need to include this bundle in scripts in your html file and load whole snark js bundle even (if you using certain functions)
    2. Developer experience wish to be better because this approach requires to operate with snarkjs lib as property of window object

Advantages of using ES module build:

  1. No additional manipulations with NodeJS dependenices from bundler perspective (Webpack, Esbuild, Vite etc) in case you want to use this package in the browser. Simple install npm i ffjavascript and you can easily use it for example in Angular or React
  2. Support of Tree shaking

What is changed?

  • Updated Rollup
  • Create Rollup config to assemble ES module for browser
  • Update package.json to use browser.esm.js in case of browser usage

What is next

UDP: in theory issue #29 should be fixed by this PR

@Kolezhniuk Kolezhniuk requested review from jbaylina, phated and demonsh and removed request for demonsh October 6, 2023 16:12
@Kolezhniuk Kolezhniuk marked this pull request as ready for review October 6, 2023 16:14
@phated
Copy link
Contributor

phated commented Oct 8, 2023

A browser bundle is unnecessary here. Perhaps you want to remove process.browser checks so the modules work directly in bundlers. Alternatively, we can use the exports section to switch specific files between a nodejs implementation and a non-node version.

@phated phated closed this Oct 8, 2023
@Kolezhniuk
Copy link
Contributor Author

@phated

A browser bundle is unnecessary here. Perhaps you want to remove process.browser checks so the modules work directly in bundlers. Alternatively, we can use the exports section to switch specific files between a nodejs implementation and a non-node version.

Hello!
Let me explain more clearly what also has been fixed in this PR:

  1. Support clear ES module build directly for browser (without node os crypto dependencies)
  2. Added check https://github.com/iden3/ffjavascript/pull/127/files#diff-0462ab1425fcb10bd181dc4f7513067635628058d4c1d851666e2b10a64ec2f4R98
  3. Added ability to run multi theading mode for some frontend frameworks (Angular, AngularJS, Ember.js, JQuery...)
    which is currently failing

Could you please explain in more detail why you have concerns about not having a browser bundle?

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.

A browser bundle is not required. Node 20+ has the global Crypto API, so we should remove the process.browser usage and remove any node-specific imports.

@OBrezhniev
Copy link
Member

Browser bundle is built anyway in SnarkJS, this PR simplifies life for people using ffjavascript directly in the browser, and unblocks creation of SnarkJS build that works fine with Angular and other frontend frameworks.

Nodejs compatibility with the browser crypto api is nice, but wasn't in the scope of this PR. And process.browser workaround is still needed for switching between os.cpus().length and navigator.hardwareConcurrency.

@jbaylina jbaylina merged commit 84267d3 into master Oct 26, 2023
25 checks passed
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.

6 participants