-
Notifications
You must be signed in to change notification settings - Fork 30
Intl types: simplify bindings for constructors / functions with optional arguments #198
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
Conversation
I like this, and this is a conversion we should do for more APIs. As in combine variants that can be a single function with optional args now with v11. We should optimize the compiler to not print the undefineds unless needed. There's an issue for it somewhere and me and @cristianoc have been chatting about it too before. |
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.
Looks great to me!
@new external makeWithOptions: (@as(json`undefined`) _, options) => t = "Intl.Collator" | ||
|
||
@val external supportedLocalesOf: array<string> => t = "Intl.Collator.supportedLocalesOf" | ||
@new external make: (~locales: array<string>=?, ~options: options=?) => t = "Intl.Collator" |
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.
An alternative here could be:
@new external make: (~locales: array<string>=?, ~options: options=?) => t = "Intl.Collator" | |
@new external make: (array<string>, ~options: options=?) => t = "Intl.Collator" |
as passing an empty array is effectively the same This would be more convenient if the common case is to pass one or more locales, but less so if it the default is more common. Not sure which is though.
3a8757f
to
8a425e7
Compare
Extended to the remaining |
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.
Great! Merge away.
a319c66
to
40431ec
Compare
Now that we can have functions with optional arguments without a trailing
()
in ReScript 11 uncurried mode, bindings can be simplified to a singlemake
function (and the same forsupportedLocalesOf
) with nice ergonomics.I also think we can drop the distinction between passing a single locale and an array of locales. Just pass an array always.
One minor downside are trailing
undefined
params in the JS output. This is probably something we should improve in the compiler, but it shouldn't do any harm here.@zth @illusionalsagacity @glennsl For now I just did it for
Intl.Collator
. If this looks good to you, I would extend it to the other types.(The first commit just removes duplicate build artifacts that were in
test/intl
plustest/Intl
).