ryyppy / rescript-promise

Proposal for a proper Js.Promise binding
127 stars 14 forks source link

Adding a "catchU" function? #11

Open johnridesabike opened 3 years ago

johnridesabike commented 3 years ago

Since catch can't take advantage of the @uncurry annotation, would it make sense to include an uncurried version?

let catchU: (t<'a>, (. exn) => t<'a>) => t<'a>

I doubt performance is a concern when catching exceptions, but it may still be nice for users concerned with JS bundle size or readability.

ryyppy commented 3 years ago

yeah that sounds like something we want

ryyppy commented 3 years ago

@johnridesabike I just tried to implement that, and I think we are getting into a situation where the benefit is not entirely clear to me:

So adding a catchU inside src/Promise.res will indeed eliminate the Curry._1 call from the catchU output, but the dependency to Curry will still exist within the module (since it sits right next to catch). From a user's POV, the curry will not show up in the actual Promise.then code (since it will be hidden in the Promise module).

Are bundlers clever enough to understand that Curry is not being used within the catchU function to remove Curry from the bundle, in case you are only utilizing Promise.catchU?

I originally thought your issue was that Curry calls are showing up in the userspace code.

johnridesabike commented 3 years ago

Yeah, my motivation came from writing a small script and bundling it (with esbuild), and looking for ways to minimize the output as much as possible. Curry._1 doesn't show up in userspace, but it will still be in the bundle.

Are bundlers clever enough to understand that Curry is not being used within the catchU function to remove Curry from the bundle, in case you are only utilizing Promise.catchU?

They should be able to. With tree-shaking, they should only include the values actually used, not the entire modules. If you bundle a file that just calls Belt.Array.mapU, for example, it will only bundle your code and the mapU function, nothing else.

Although, I can see that your point that the benefits to this is hard to measure. Any non-trivial app will probably end up using the Curry._1 function anyway. And since the uncurrying only has to execute in the event of an error, and only once (compared to something like Array.map which would execute for every array item), then the performance may be negligible.

ryyppy commented 3 years ago

Although, I can see that your point that the benefits to this is hard to measure. Any non-trivial app will probably end up using the Curry._1 function anyway. And since the uncurrying only has to execute in the event of an error, and only once (compared to something like Array.map which would execute for every array item), then the performance may be negligible.

So the Curry._1 call will definitely be in the output and it really boils down to the question if shaving off these few bytes might be worth it. I've got the commits ready for adding this API, so if you have a production use-case that really needs that change, we could add it.

Alternatively, I could imagine that there's just a fraction of users highly relying on removing all occurrences of Curry calls, so there's always the option to add an adhoc binding in an extended Promise module, even if we decide not to add it.

johnridesabike commented 3 years ago

After thinking about it some more, I don't think I have a use-case where catchU would make a measurable difference.

It may still be useful to add (or perhaps not) but either way I doubt I would need it.