Support valid min, max, step and value types for various input types

[sgny]

For various types of <input />, there are different allowed types for various props (except string, which is the fallback). To allow some degree of type safety, while not excluding any valid use, this PR defines multiple props, all rendered as the same prop (e.g. min) by the [@bs.as] annotation, but accepting different types such as int, float, etc.

This is not exhaustive as more props could be similarly defined. I have chosen the string fallback type as a sort of default, where using the canonical JS prop name in Reason requires a string argument.

[rickyvetter]

I don't think supporting these unsafe uses is a good idea. If BuckleScript supports @bs.unwrap on @bs.deriving abstract or (much preferred imo) @bs.as on @bs.obj then we can do these safely. I'd rather get support for that and use it.

[yawaramin]

I agree it's not ideal that people can create these objects with maxDate, maxInt, etc. but I think it's done for backward-compatibility. If we change the type of max it's a breaking change. If that is preferred it would probably happen on RR 1.0.0, right?

[rickyvetter]

Next release will be breaking at 0.8.0 (because NPM semver is odd), so no worries there. I'd prefer a safe and breaking change to this for sure. But I don't think a safe api can be made for it at the moment, which is why I point to potential BuckleScript changes. I would love nothing more than to be proven wrong about a safe api that's available to day though :)

[yawaramin]

If we're open to breaking changes, then we can use an abstract type today with zero runtime (the 'union type trick'). E.g.:

module Input = {
  type t;

  external string: string => t = "%identity";
  external date: Js.Date.t => t = "%identity";
  external float: float => t = "%identity";
  external int: int => t = "%identity";
};

[@bs.deriving abstract]
type domProps = {
  ...,
  [@bs.optional] max: Input.t,
  [@bs.optional] min: Input.t,
  [@bs.optional] value: Input.t,
  ...
};

EDIT: usage would look like,

module Input = ReactDOMRe.Input;

<input value=5->Input.int min=0->Input.int max=10->Input.int>

[sgny]

I had chosen this setup to prevent breaking and I find it more straightforward to avoid multiple functions (at the risk of the user passing multiple props rendering to the same name), but in the absence of @bs.unwrap, what @yawaramin proposed is the only truly safe way to handle this.

@yawaramin Should I revise the PR or would you like to handle that?

[yawaramin]

I think I just found a slightly safer approach:

module Input = {
  type t('a);

  external string: string => t(string) = "%identity";
  external date: Js.Date.t => t(Js.Date.t) = "%identity";
  external float: float => t(float) = "%identity";
  external int: int => t(int) = "%identity";
};

[@bs.deriving abstract]
type domProps('a) = {
  [@bs.optional] min: Input.t('a),
  [@bs.optional] max: Input.t('a),
  [@bs.optional] step: Input.t('a),
};

This should force the related attributes: min, max, etc., to be of the same type. So it should be impossible to do <input value="howdy"->Input.string min=0->Input.int>.

I'm not sure if Reason JSX will work with this though. Let me try both approaches tonight and update this PR with the one that works.

[yawaramin]

I've proposed https://github.com/sgny/reason-react/pull/1

[yawaramin]

@sgny oh now I understand what you did: dateStep: int => step(Js.Date.t). Fantastic!

[sgny]

@yawaramin

We could probably only allow float for step. However, given "any" requires special handling, we might as well handle step properly. With a submodule for Step, we'd have:

module Input = {
  type t('a);
  external string: string => t(string) = "%identity";
  external date: Js.Date.t => t(Js.Date.t) = "%identity";
  external float: float => t(float) = "%identity";
  external int: int => t(int) = "%identity";

  module Step: {
    type t('a);

    [@bs.inline "any"] let any: t(_);

    external date: int => t(Js.Date.t) = "%identity";
    external float: float => t(float) = "%identity";
    external int: int => t(int) = "%identity";
  } = {
    type t('a) = string;

    [@bs.inline] let any = "any";

    external date: int => t(Js.Date.t) = "%identity";
    external float: float => t(float) = "%identity";
    external int: int => t(int) = "%identity";
  };
};

[yawaramin]

@sgny yup, perfectly valid. I love how the type of the 'input type parameter propagates backwards and forwards among the min, max, value, and step attributes.

[sgny]

@yawaramin

Thinking a bit further to make sure nothing is missed, I realized value might also be bool and I added an appropriate function.

We could use @bs.inline to also enforce appropriate value for type_ as below:

module Input: {
  type type_('a);
  type t('a);
  type step('a);

  external string: string => t(string) = "%identity";
  external date: Js.Date.t => t(Js.Date.t) = "%identity";
  external float: float => t(float) = "%identity";
  external int: int => t(int) = "%identity";
  external bool: bool => t(bool) = "%identity";

  [@bs.inline "number"]
  let intType: type_(int);
  
  [@bs.inline "number"]
  let floatType: type_(float);

  [@bs.inline "date"]
  let dateType: type_(Js.Date.t);

  [@bs.inline "checked"]
  let checkedType: type_(bool);

  external otherType: string => type_(_) = "%identity";

  [@bs.inline "any"]
  let anyStep: step(_);

  external dateStep: int => step(Js.Date.t) = "%identity";
  external floatStep: float => step(float) = "%identity";
  external intStep: int => step(int) = "%identity";
} = {
  type type_('a) = string;  
  type t('a);
  type step('a) = string;

  external string: string => t(string) = "%identity";
  external date: Js.Date.t => t(Js.Date.t) = "%identity";
  external float: float => t(float) = "%identity";
  external int: int => t(int) = "%identity";
  external bool: bool => t(bool) = "%identity";
  
  [@bs.inline]
  let intType = "number";
  
  [@bs.inline]
  let floatType = "number";
  
  [@bs.inline]
  let dateType = "date";

  [@bs.inline]
  let checkedType = "checked";

  external otherType: string => type_(_) = "%identity";
  
  [@bs.inline]
  let anyStep = "any";

  external dateStep: int => step(Js.Date.t) = "%identity";
  external floatStep: float => step(float) = "%identity";
  external intStep: int => step(int) = "%identity";
};

Since type_ does not only relate to input, perhaps moving it outside Input might make sense, but would this make the whole thing too unwieldy?

[rickyvetter]

How does this work if you don't define any of the parameterized values? For example <div key="myDiv" /> - does this have an unknown/generalizable type '_a?

Thoughts on this in relation to https://github.com/reasonml/reason-react/pull/571? I believe it's somewhat controversial but I would like to further specialize the props types per-element. Since many elements don't even take these attributes it would minimize the places we would need to add type parameters for this kind of tying.

In general I am into this change and the implementation looks really sweet. I think we should be careful with it though as it adds a lot of complexity. For example you could imagine doing the same with color where folks can't pass strings as colors and instead are force to do something like Color.rbga(255, 255, 255, 0.5); and then all of a sudden we need to validate <=255 at the type level and things get out of control and we end up with a props validation runtime thats 100s of lines long.

[yawaramin]

Hi @rickyvetter I tried my local version and didn't get any errors, e.g. let input = <input type_="number" />;. I think this is because of [@bs.deriving abstract]'s type representation as an immutable type.

You're right there is complexity here, but I was thinking the tradeoff is worth it because as far as I know the interplay between the min, max, value, and step attributes is unique among DOM element attributes. If and when we do move to the proposed capitalized elements with their own prop types, these more specialized types would migrate over to the Input element I guess.

The other issue is that, if we lock down DOM element props using a codegen technique (say, from the W3C specs), then the codegen would need special handling logic for these special input validations that we're putting in place here. If you indeed have that in your sights, then we'd probably want to fall back to the less type-safe option I proposed before, https://github.com/reasonml/reason-react/pull/442#issuecomment-620335223

[sgny]

@rickyvetter

Sorry, for the late response, I've been tied down for the past few days with work.

I'd already tried only passing props without type parameters and as with @yawaramin had no issues. This PR already has limited impact when the component is not input, but perhaps we can minimize it further.

I'll prepare a commit specifying type domProps('value) where value: 'value so that the Input module becomes relevant only when min, max, step props are actually used. I believe defining type_: type_('value) would not cause much impact on users, but would have type safety benefits.

[sgny]

I have implemented type_: type_('value) with a few example values but given the huge number of valid values, perhaps it might be more straightforward to define type_: string.

@rickyvetter We have highly specific bindings in reason-react-native, but even then there are a lot of intricacies such as platform-dependent props or prop values, props that should be set in conjunction, etc. that we do not enforce. To what extent will the new component definitions enforce correctness? Will it happen with component definitions in this repo?

[rickyvetter]

I consider this closely related to #567. I'm going to do some more work there and don't want to land this preemptively if it requires more breaking changes for elements that won't be parameterized.

[davesnx]

I'm closing this, but I might ping @sgny or @yawaramin in further development since the idea to keep a higher bar for type-safety for lower-case components has been under my interest for a while.