[RFC] Add support for optional flow types

[dwwoelfel]

Adds support for optional types. Only supports optional values, not optional params.

With this diff

type maybeTypes = {
  a: ?string,
  b?: string,
  c?: ?string
}

will generate

type maybeTypes = Js.t {.. a : Js.null_undefined string, b : string, c : Js.null_undefined string };

but I think it should generate b as Js.undefined string.

It breaks external function declarations. I could use a pointer to how I can distinguish between a type and an external declaration.

Example:

declare function someFunction(a?: string): string

before this diff, would generate something like

external someFunction : a::string? => string

now it generates something like:

external someFunction : a::Js.null_undefined string? => string

PR Checklist

• [ ] Corresponding issue: #0000
• [X] Formatted code with refmt
• [X] Ran tests npm test and updated snapshots

[bbqbaron]

Hi! I'm not Ryan but thought I'd stop by in case we can both learn something. Here's my thought process from skimming the code:

My guess is there's a gap between Flow's optional keys and the Bstype.t type used to represent the Bucklescript side of things. I'm looking here where we define Object as holding (list (string, t)). To me, that says that we don't recognize/remember whether Flow represented any of the object's props as optional, so we wouldn't be able to print the code correctly.

What I'd probably do is to change that type somehow. The most immediate option though less readable is Object (list (string, bool, t)), where bool is whether it's optional or not.

Then, we need to get that info from Flow decls, maybe around here.

Finally, we print code for optional props differently, to get the results you want above.

I'm assuming here that we want to be precise/pedantic and understand that optional keys are technically different from nonoptional keys with optional values (which basically boils down to using Js.undefined for optional keys, AFAICT). I could be wrong and we might not really care about the distinction, but I figure for large-scale automatic conversions it's probably best to be exact :).

Does that help? Does my reasoning seem on-point?

[rrdelaney]

Hey @dwwoelfel! Thanks so much for this PR 😄 Hope everything went smoothly when developing it!

This is a great change, and anything that can get us closer to generating the exact types is a huge step forward. I do think that @bbqbaron is right, we should be generating Js.null_undefined for nullable types, as in the Flow documentation a maybe type can be null, undefined, or the value. An optional field however should be Js.undefined, because with Flow an optional field can only be undefined, null is a valid value.

This introduces the need for another type! Right now we put optional fields and maybe types into BsType.Optional t, but the use cases here point out that we could also have BsType.Nullable t. BsType.Nullable would be for nullable values that can be either undefined, null, or the value. So where we put BsType.Optional now would become BsType.Nullable, and then the optional fields would become BsType.Optional. Would that make sense? I think with the Nullable and Optional split the function output should be fixed as well!

I think @bbqbaron is correct in saying we should be precise 😃 The unofficial stance of ReasonablyTyped is to be precise*, rather than shaky. A passing test should mean the output has the same semantics as the input.

(* to an acceptable degree within a reasonable amount of time)

I'd love to help out on doing this! Feel free to hit me up on Discord to work through any problems!

[rrdelaney]

Closing because ReasonablyTyped is going through a substantial rewrite