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

Usage of Printexc increases the bundle size quite a bit #39

Closed
ozanmakes opened this issue Jan 5, 2019 · 8 comments
Closed

Usage of Printexc increases the bundle size quite a bit #39

ozanmakes opened this issue Jan 5, 2019 · 8 comments
Milestone

Comments

@ozanmakes
Copy link

I'm having a great experience with Repromise, but had to give up on it for a project which aims for the smallest bundle size that's easily achievable. I noticed only Repromise depends on CamlinternalFormat in this particular case, and switching back to Js.Promise resulted in a much smaller bundle.

Is this dependency easily avoidable in Repromise when targeting BuckleScript?

I created a Gist with rollup-plugin-analyzer to demonstrate where most of the 41k bundle (minified) comes from: https://gist.github.com/osener/91dd9838200d53c0b3bf824a9394885c

You can try it out by running

$ git clone https://gist.github.com/91dd9838200d53c0b3bf824a9394885c.git repromise_bundle_size_test
$ cd repromise_bundle_size_test
$ npm install
$ npm run build
@aantron
Copy link
Owner

aantron commented Jan 5, 2019

Yes, it should be possible to avoid. I seem to have assumed that most projects using Repromise will also somehow pull in Printexc, but I guess that's not the case. If you have a chance (and have not done this already), could you check if removing the call to Printexc.to_string here gives the size results you are looking for?

If so, we can try to remove it in multiple ways. Perhaps we can print the exception using console.error by default. In the docs, we can more strongly encourage developers to set an async exception handler that uses some dependency that the developer is explicitly willing to take on.

@ozanmakes
Copy link
Author

@aantron indeed, that single line is the offender. It cuts the minified bundle size from 41k down to 2.3k. Next largest one is es6/curry but I don't think it's necessary to eliminate (except for performance reasons).

BuckleScript's author recommends against using printf and others for similar reasons: rescript-lang/rescript#482

image

@ozanmakes
Copy link
Author

Using Js.Console.error will cause the exception to look something like this in the console unless this is set up (and that will only work in develelopment mode):

[ [ 'Failure', -2, tag: 248 ], 'message' ]

If this is acceptable I can open a PR. I don't know a way to show OCaml data structures in BuckleScript any other way.

@aantron
Copy link
Owner

aantron commented Jan 5, 2019

I think that's acceptable, and would be happy to merge a PR for it. We should probably prefix it with "exception," or something like that. I also thought it might be a good idea to give instructions on what the developer can do to get a proper message, but I'm not sure if that will be good post-deployment.

@aantron aantron added this to the 0.6.2 milestone Aug 16, 2019
aantron added a commit that referenced this issue Sep 8, 2019
@aantron aantron closed this as completed in 0bd97a3 Sep 8, 2019
@aantron
Copy link
Owner

aantron commented Sep 8, 2019

I removed usage of Printexc and replaced it with Js.Console.error. The bundle size has decreased from 120k to 21k. The increase to 21k is because we added some functions that depend on array in #40. I'll see about fixing that next.

I added a bundle size test, which is basically just

https://github.com/aantron/repromise/blob/0bd97a3d10225f54b445c2982973a94684931119/test/bundle/main_repromise.js#L1-L3

This is built with webpack in CI:

-rw-rw-r-- 1 travis travis 5003 Sep  8 14:46 lib/js/src/js/repromise.js
-rw-rw-r-- 1 travis travis  1044 Sep  8 14:46 test/bundle/bundle_control.js
-rw-rw-r-- 1 travis travis 21307 Sep  8 14:46 test/bundle/bundle_repromise.js

So we can easily notice if it starts growing too much again.

aantron added a commit that referenced this issue Sep 8, 2019
aantron added a commit that referenced this issue Sep 8, 2019
@aantron
Copy link
Owner

aantron commented Sep 8, 2019

I also got rid of Array and Curry. Repromise now adds only 2.5K to bundle size.

@ozanmakes
Copy link
Author

Awesome! Apologies for dropping the ball on this.

@aantron
Copy link
Owner

aantron commented Sep 9, 2019

No worries, I dropped the ball, too :) I'm catching up on all the issues in the repo, and will release this in a 1.0.0 release soon :)

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

No branches or pull requests

2 participants