Stricter Intl Bindings

[illusionalsagacity]

resolves #63 and #64

gonna write some more test coverage later

• Intl.NumberFormat.Grouping.t needs a parse for resolvedOptions
• remove redundant localeMatcher copy pasta
• remove oneTo21 zeroTo20 in NumberFormat, use the common one instead

[zth]

@illusionalsagacity looking like you're making great progress!

[illusionalsagacity]

@illusionalsagacity looking like you're making great progress!

Yeah the bulk of the API surface should be bound with what's there, could use some style review to make sure it matches the rest of the stdlib's intent.

Also was it intentional before that some features not supported by Firefox were left out?

[zth]

I don't know whether it was intentional. What do you think about them being left out, should they be in your opinion?

[illusionalsagacity]

I don't know whether it was intentional. What do you think about them being left out, should they be in your opinion?

I lean towards including them, and letting the end user decide about when to use something or not. I did mark some things that had limited support in the records as optional however, because typing as a record and omitting the property makes it entirely unavailable to the platforms that do support it.

[illusionalsagacity]

Obviously this is a fairly large pull request, I'd be happy to split it up if that helps reviewing it.

Could use some input from others who have used most of these, my personal experience is mostly with Intl.DateTimeFormat.

[cknitt]

@illusionalsagacity I was working on modernizing my react-intl bindings and wanted to base them on RescriptCore.Intl.DateTimeFormat when I realized that that module still needed to be fleshed out. Started doing that myself before I noticed that you had actually already created this PR. 🤦

Would really like to move this PR forward!

@zth Some general questions:

1. Do we need to keep supporting ReScript 10 / curried in Core? If we go ReScript 11 + uncurried only, we have more options for the bindings - see 3. below.

2. Do we want to use polymorphic variants here when the IMHO more ergonomic normal variants would also do the trick?

E.g.

type dateStyle = [#full | #long | #medium | #short]

vs.

type dateStyle =
  | @as("full") Full
  | @as("long") Long
  | @as("medium") Medium
  | @as("short") Short

3. For constructors, instead of providing multiple variants like

@new external make: unit => t = "Intl.DateTimeFormat"
@new external makeWithLocale: string => t = "Intl.DateTimeFormat"
@new external makeWithLocales: array<string> => t = "Intl.DateTimeFormat"
@new external makeWithLocaleAndOptions: (string, options) => t = "Intl.DateTimeFormat"
@new external makeWithLocalesAndOptions: (array<string>, options) => t = "Intl.DateTimeFormat"
@new external makeWithOptions: (@as(json`undefined`) _, options) => t = "Intl.DateTimeFormat"

it would now be possible to have a single one like this:

@unboxed
type localesParam =
  | String(string)
  | Array(array<string>)

@new external make: (~locales: localesParam=?, ~options: options=?) => t = "Intl.DateTimeFormat"

or maybe just

@new external make: (~locales: array<string>=?, ~options: options=?) => t = "Intl.DateTimeFormat"

What approach should we prefer?

[zth]

1. I think we'll need to support both v10 and v11, and curried + uncurried for a while. What'll make it into the compiler in v12 is uncurried obviously though. I think that as soon as v11 drops, we can go ahead and have 2 branches and releases of Core. One for v10 + v11 curried, and one that's dedicated to v11 uncurried and that'll be what'll be merged into the compiler stdlib for v12.
2. Definitely regular variants where it makes sense now that we can use them given the flexible runtime representation. That should definitely be the recommended way to do things. Although using polyvariants for simple values doesn't hurt, we should prefer regular variants whenever possible.

This way, we can do some fairly aggressive changes to Core for v11+ and uncurried (migration will be easy to do a migration script for just like now) and make the APIs as nice as possible for an uncurried world.

[cknitt]

I think that as soon as v11 drops, we can go ahead and have 2 branches and releases of Core. One for v10 + v11 curried, and one that's dedicated to v11 uncurried and that'll be what'll be merged into the compiler stdlib for v12.

@zth Ok, so it's probably best to wait for the v11 uncurried / v12 branch with this PR? So that we can use regular variants and all the other v11 goodness.

What are your thoughts regarding my suggestion for the make binding?

@new external make: (~locales: array<string>=?, ~options: options=?) => t = "Intl.DateTimeFormat"

[cknitt]

It would be good to have a separate PR with the fix for #64 to merge now though.

[illusionalsagacity]

I think that as soon as v11 drops, we can go ahead and have 2 branches and releases of Core. One for v10 + v11 curried, and one that's dedicated to v11 uncurried and that'll be what'll be merged into the compiler stdlib for v12.

@zth Ok, so it's probably best to wait for the v11 uncurried / v12 branch with this PR? So that we can use regular variants and all the other v11 goodness.

What are your thoughts regarding my suggestion for the make binding?

@new external make: (~locales: array<string>=?, ~options: options=?) => t = "Intl.DateTimeFormat"

IMO, the multiple make functions is probably more in line with what a ReScript <=10 consumer would expect, and to me it makes sense to leave those kind of changes to take advantage of the ReScript 11 functionality to that branch/versions as the consumer will likely be dealing with other libraries making similar changes at that time.

[glennsl]

2. Do we want to use polymorphic variants here when the IMHO more ergonomic normal variants would also do the trick?

What makes normal variants more ergonomic? All else being equal, I find polymorphic variants more predictable and closer to the guiding principle of familiarity for JS developers. If I look at MDN and see that I need "best-fit", it's pretty simple and predictable to translate that into #"best-fit". Normal variants are a bit more flexible though, which is useful in at least one case here where strings are weirdly mixed with a bool value.

(also it's a lot more verbose and in my own bindings I'm lazy, so that's the style that would be more consistent with my own codebase)

[zth]

2. Do we want to use polymorphic variants here when the IMHO more ergonomic normal variants would also do the trick?

What makes normal variants more ergonomic? All else being equal, I find polymorphic variants more predictable and closer to the guiding principle of familiarity for JS developers. If I look at MDN and see that I need "best-fit", it's pretty simple and predictable to translate that into #"best-fit". Normal variants are a bit more flexible though, which is useful in at least one case here where strings are weirdly mixed with a bool value.

(also it's a lot more verbose and in my own bindings I'm lazy, so that's the style that would be more consistent with my own codebase)

There are definitively upsides to polyvariants compared to regular variants, like inline type definitions, 1:1 between what you write and what you get, etc. I however think that these things from regular variants make them more ergonomic:

• Regular variants can have docstrings. Important for the devex of especially certain bindings
• You never have to escape regular variants like you may need to do with polyvariants. This of course has the downside of the thing you write not being 1:1 to what it compiles to. But polyvariants also need prepending #, so there's downsides to both
• You can mix more underlying primitives with (untagged) regular variants than in polyvariants
• Static analysis exists for regular variants via reanalyze, but it doesn't for polyvariants. "Are these constructors ever used?" etc. Less important in bindings where you want to cover the entire API surface regardless of if it's used of course
• Regular (untagged) variants can have "any string/int/float" cases without special effort, which is often convenient for interop: https://rescript-lang.org/docs/manual/latest/variant#advanced-catch-all-constructors

Some thoughts.

[cknitt]

In addition to the points mentioned by @zth, my concern with polymorphic variants is this: When writing bindings, it is highly likely that you will end up using untagged (regular) variants somewhere. If you are also using polymorphic variants in other places, then this mixed usage may feel weird/inconsistent to the user of the bindings, compared to just using regular variants everywhere.

Therefore I feel that we should promote the latter as best practice. (Good example: our ReScript MUI bindings.)

[cknitt]

In any case, we should proceed with this PR now that master is v11+ only. 🙂

@zth We could also rebase and merge it quickly and, if we decide to do the adaptations I mentioned in #65 (comment), do them in separate PRs. What do you think?

[glennsl]

If you are also using polymorphic variants in other places, then this mixed usage may feel weird/inconsistent to the user of the bindings, compared to just using regular variants everywhere.

I think it makes sense to distinguish simple "literal" enum data types, such as unions of strings where inlining the type makes more sense than defining a separate nominal type, from more complex variants with payloads ad objects and such. Ideally I'd like to see a bit more intuitive syntax for the different types of primitives, and support for booleans as well, as I think that aligns quite well with TypeScript idioms. I even use this in non-binding situations for things that map to class names, for example, such as the size and style of a component. This distinction feels very natural and intuitive to me.

Right now, though, the big problem here is the one case where string and bool values are mixed, that would have to be special-cased. That would be avoided by using regular varaints, but at the expense of making the whole API more clunky IMO.

[zth]

@cknitt @glennsl I'm happy with whatever direction you come up with here. I'm a bit short on time right now but I think moving forward with this is important, so please feel free to move it forward in the way you see fit best.

[cknitt]

Ok, then @illusionalsagacity could you rebase the PR on the latest master, get the tests to run and add a changelog entry?

Then we can merge this, and make any of the adaptations I mentioned in #65 (comment) later in separate PRs.

[cknitt]

Looks great! Thanks again!