aantron / promise

Light and type-safe binding to JS promises
MIT License
340 stars 24 forks source link

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

Closed ozanmakes closed 5 years ago

ozanmakes commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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: https://github.com/BuckleScript/bucklescript/issues/482

image

ozanmakes commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ozanmakes commented 5 years ago

Awesome! Apologies for dropping the ball on this.

aantron commented 5 years ago

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 :)