-
Notifications
You must be signed in to change notification settings - Fork 6
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
Entry point and bundling changes #368
Conversation
size-limit report 📦
|
I'll need to review how the import checks are done too, since I changed the entry points. Edit: I simply forgot about updating the github actions and run a required |
cd07933
to
5d92a15
Compare
5d92a15
to
c099476
Compare
…dencies" This reverts commit c0159a7.
443428d
to
d6fe8a1
Compare
Set back to draft because I've broken something related to types that I need to check. |
f9a6ac4
to
d4c8f10
Compare
Ok this is an issue I didn't count on. The problem here is the SDK is using I also found an issue with how the exports for protobuf where being treated, that's why I created a new dvote-proto version and updated it here in this PR. |
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.
Did anonymous and more E2E tests and everything seems to work fine.
Congrats for the great result.
LGTM.
This PR leaves the bundle exactly identical as the one we're currently generating, Now the bundle is a 98% of its original size! And also this PR brings some improvements to the way we were bundling the SDK:Bundling changes
The
external
packages are now all taken from package.jsondependencies
field. The only exception is@vocdoni/proto/vocdhain
, since the import does not match with the field in package.json.Other than that, right now deciding if a dependency should be bundled or not is just a matter of placing it in
devDependencies
ordependencies
.Also, sourcemap generation has been set to be only done ifNODE_ENV == 'development'
, since the final npm package should not have sourcemaps. Rollup.js' documentation is not very clear about what node env is being used during--watch
mode, so right now it may be causing the same effect as having it set tofalse
.Anyway, since in this PR we change the entrypoint to be the ts files directly, there should be no need for sourcemaps.I ended up disabling sourcemaps, due to the previous comment (you're live-building the ts files, your dev server should handle these sourcemaps).
Dramatic size change
The huge size change in the bundle is due mostly to the fact that we were bundling everything in our package in order to ensure people didn't had issues, specially from browser environments (since some of the libraries used were made only with nodejs in mind). Now, some of these dependencies, like snarkjs or circomlibjs, that were bundled in the package have been added to the
dependencies
array.This causes the bundling process to ignore these packages during bundling, but also ensures people installing the SDK will properly install the dependencies as we typed them in our
package.json
file.Entry point changes
The entry point has been changed to
src/index.ts
. This is done this way because, while developing, having the typescript as an entry point allows us to link the SDK to other projects and directly make changes to the SDK without requiring us to build in-between. This, of course, requires the end-developer to be bundling with something that is typescript compatible.Since we don't need to build in-between anymore, the
prepare
npm script has been removed too.To properly have the entry points defined for the npm package, a
prepack
script callingclean-package
has been added. This script will ensure to modify the package.json file and add the entries required to access the bundled packages (exactly as we had it).Note: due to this
prepack
script, runningyarn pack
locally will result in a modified package.json that should not be committed, ever. Apostpack
script ensures to leave your package.json file like it was before the pack process.Things I think this PR may address
We had issues in the past dealing with packages like axios, and I think that was due to the confusing way we were defining external packages, and because we were (accidentally) trying to bundle it into our SDK. Axios should not be bundled into our final package, instead, it should be defined as a dependency. Doing it this way, and given the changes done to the bundling configuration, this package shouldn't give any problems whatsoever.
Note:
peerDependencies
were not considered into the bundling because we're not using them here (and I don't see a reason for it tbh, although you may see some of my commits trying related things, but ended up all removed).Forks
In order to make this new bundling work as we really wanted to, I had to fork and change how some packages where being bundled. This was required because these packages are mostly abandoned, not receiving updates since years ago, and accepting PRs with the same intensity (none).
Here's the list of packages that I had to fork and the changes I had to make to them:
buffer
so the package was able to properly build versions for both node and the browser.circomlibjs
, I had to install thebuffer
package and use it where needed.Note all forks have an
f/browser
branch, where I'm pointing to.List of checks I'd like to review if they work:
prepare
script this way)