Add one of proptype

[samwgoldman]

I noticed when digging around the source that a number of PropTypes are not supported, and thought this would be a good place to start contributing.

I tried to follow the style of the existing code, but I would appreciate any formatting critiques.

I'm not super happy with the way I'm creating the string literal EnumT, as I copied it from convert. Should this code be factored out into a utility, or is there a better way to go about this?

Hope this helps!

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[bhosmer]

Hey Sam - I think what you want to do instead here is have the helper build a map of elements to AnyTs, and then build a final EnumT with the result. That way you'll get a single EnumT<{"foo":any; "bar": any; ...}> rather than a union of singleton EnumTs.

And yeah, per your comment, I'd factor a mk_enum_type out of here and convert. It could take a list of strings, and build the key-to-AnyT map out of that.

[samwgoldman]

That does make more sense.

[samwgoldman]

@bhosmer Hope the latest commit addresses your comments. Constructing the oneOf EnumT still involves a de-re-structuring of an option type, but that seemed to me the cleanest way to bail out if we encounter something other than a string literal. I imagine some procedural code might allocate less, but this hardly seems like a hot spot.

[samwgoldman]

Found an issue while I was trying to separate the tests from the existing spread test. If the oneOf proptype isn't required, flow doesn't warn about invalid values. The required/optional/default gymnastics are a little over my head at the moment, so any guidance here is appreciated.

[bhosmer]

Hi Sam - sorry for the delay. Thanks for your update! Couple items of feedback:

• Re packing and unpacking the option value: with a quick simplification to string_literals, you can avoid doing this. Basically, if you match only on the node list rather than the (node list, result) pair, then you don't need to build the option until the end. Check it out:

     (* let rec string_literals lits es = match es with *)
     (* ...or equivalently: *)
     let rec string_literals lits = function
        | Some (Expression (loc, Literal { Ast.Literal.
            value = Ast.Literal.String lit
          })) :: tl ->
            string_literals (lit :: lits) tl
        | [] -> Some lits
        | _ -> None in
      (match string_literals [] es with
      ...

• Re the enforcement failure if .isRequired isn't specified. This is definitely a bug, but fixing it is beyond the scope of this PR - there are deeper things in play, and the problem is almost certainly not confined to oneOf. (In fact, it would be great if you could post an issue for this. :) For our purposes here, I would add isRequired to the test case, plus a comment noting why it's there.
• Aside: zooming out a bit, it's worth mentioning that the silent failure strategy that Flow takes here -silently ignoring propTypes specs if they're incorrect in any way, rather than giving errors - is suboptimal, and a usability TODO. I don't know if it strikes your fancy, but this sort of work (for instance here, an error on non-string-literals, or if the argument itself isn't an array literal) is more than fair game for future contributions, if you're interested. Not the most glamorous stuff but great for usability. :)

Anyway, if you could avoid the option repack as described above and add isRequired to your new test, I think we're good to go. And thanks again! Really awesome to have you contributing.

Basil

[samwgoldman]

Pending the test run, I think I addressed your comments. Thanks for your patience and excellent feedback with my contribution.

I am definitely interested in spending more time with this proptypes code: adding warnings for proptypes that are unchecked and adding checks to cover more proptypes are both on my list of things to look into.

[bhosmer]

Done - thanks again Sam!