FormId compare is not save

[Coobaha]

hi @alexfedoseev

I've noticed that Formality__Form.re uses Pervasives.compare:

module Make = (Form: Form) => {
  module FieldId =
    Id.MakeComparable({
      type t = Form.field;
      let cmp = Pervasives.compare;
    });
  ...
}

it will throw error in runtime if Form.field is complex type that has function or any other non-comparable type)

Maybe it should be something like this:

module type Form = {
  type field;
  type state;
  type message;
  type submissionError;
  let validators: list(Validation.validator(field, state, message));
  module FieldId:
    { type identity; type t = field; let cmp: Id.cmp(t, identity); };
};

module Make = (Form: Form) => {
  module FieldId = Form.FieldId;
  ...
}

[alex35mil]

Hey @Coobaha, sorry, I missed this issue since it's doesn't get to my personal feed anymore.

Can you elaborate on the use case? module FieldId would add quite a lot to public api: in terms of boilerplate and complexity. And I expect it'd be confusing for some users. So I'm not sure if it's worth it.

[Coobaha]

@alexfedoseev no worries! :)

I guess it is possible to do without breaking change? by introducing 2functors, one is wrapper for current Form interface that will add current FieldId and another that will use custom FieldId provided by consumer.

My use case:

I use re-formality to render dynamic forms with dynamic validations.

A bit more details:

my Form.state is Belt.Map.t(Meta.Id.Cmp.t, FormField.t, Meta.Id.Cmp.identity)

and FormField.t is GADT

  type fieldType(_) =
    | Boolean: fieldType(bool)
    | String: fieldType(string)
    | Iban: fieldType(Iban.t)
    | Gender: fieldType(gender)
    | Address: fieldType(string);

  type t =
    | NotSupported(Meta.Id.t, string)
    | Field(fieldType('a), option('a), Meta.t(option('a))): t;

fields and validators inside Formality__Form.re are

    input: Form.state,
    fields:
      Map.t(Form.field, Validation.status(Form.message), FieldId.identity),
    validators:
      ref(
        Map.t(
          Form.field,
          Validation.validator(Form.field, Form.state, Form.message),
          FieldId.identity,
        ),
      ),

So i have complex fields, comparing them structurally is not very efficient (it was not even working because same fields with different values are not equal structurally), but main problem is that it is also unsafe because of Pervasives.cmp

I am using patched version at the moment and it works nicely

[alex35mil]

@Coobaha AFAIU you suggest to turn current Make functor into a thin wrapper around underlying MakeWithFieldId functor (or something like that) that accepts custom FieldId. Mind to submit a PR?

[Coobaha]

@alexfedoseev yeap, will do

[Coobaha]

@alexfedoseev please check #53