shakacode / re-formality

Form validation tool for reason-react
https://re-formality.now.sh
MIT License
244 stars 35 forks source link

Warnings 40 and 42 with onSubmit callback #62

Closed johnridesabike closed 4 years ago

johnridesabike commented 4 years ago

If I have code like this:

let form =
  useForm(
    ~initialState=initialState,
    ~onSubmit=(state, form) => {
      /* do something with state here */
      form.notifyOnSuccess(None);
      form.reset();
    },
  );

I get these warnings:

src/App.re|180 col 16 warning| Warning 40: notifyOnSuccess was selected from type Formality__FormWithId.Validation.submissionCallbacks. It is not visible in the current scope, and will not  be selected if the type becomes unknown.
src/App.re|180 col 16 warning| Warning 42: this use of notifyOnSuccess relies on type-directed disambiguation, it will not compile with OCaml 4.00 or earlier.
src/App.re|181 col 16 warning| Warning 40: reset was selected from type Formality__FormWithId.Validation.submissionCallbacks. It is not visible in the current scope, and will not  be selected if the type becomes unknown.
src/App.re|181 col 16 warning| Warning 42: this use of reset relies on type-directed disambiguation, it will not compile with OCaml 4.00 or earlier.

I can get around it by doing this:

form.Formality__Validation.notifyOnSuccess(None);
form.Formality__Validation.reset();

Which is kind of ugly, but I can also do this:

open! Formality__Validation;
form.notifyOnSuccess(None);
form.reset();

Note that using open instead of open! will also cause this warning:

src/App.re|177 col 40 warning| Warning 45: this open statement shadows the label reset (which is later used)

Unless I'm missing something, all of these options feel hacky and aren't intuitive. I'm not familiar with the internals of re-formality, but my general suggestions are:

alex35mil commented 4 years ago

@johnridesabike In the next major version all types are defined in the Formality module. AFAIU it should address this issue, right?

And if you're just starting to use the lib, it makes sense to switch to the next api.

I'm almost finished with the docs: https://github.com/MinimaHQ/re-formality/tree/ppx/docs And published RC: re-formality@next

johnridesabike commented 4 years ago

Thanks! I just installed the new PPX version and it looks really nice. It does fix this specific issue, since those fields are simply namespaced with Formality now.

But the new version has created a whole new problem. Any time I access a field in MyForm.input (e.g. form.input.username) I get warning 42:

Warning 42: this use of username relies on type-directed disambiguation, it will not compile with OCaml 4.00 or earlier.

I'd rather not turn warning 42 off, since I think it's useful in general.

alex35mil commented 4 years ago

AFAIU warning 42 exists for backcompat. Not sure if it's useful in context of BuckleScript.

johnridesabike commented 4 years ago

That makes sense. I'll keep it turned off for this project for now.

I didn't see if the new docs mention warning 42, but it's probably a good idea to tell users to turn it off. Especially since there are guides out there advising people to turn it on (like https://dev.to/yawaramin/ocaml-reasonml-best-practice-warnings-and-errors-4mkm).

Thanks for the help, and good luck with the new version!

alex35mil commented 4 years ago

I didn't see if the new docs mention warning 42, but it's probably a good idea to tell users to turn it off.

Indeed. I'll add it tomorrow. Thanks for suggestion and let me know if you have any questions!