Wrong component type signature when using type annotation #85

[davesnx]

This PR fixes https://github.com/ml-in-barcelona/jsoo-react/issues/85 the type signature for parametrized types when they are being used in labelled arguments. Don't fully know why we had that but was restructuring the parametrized type when option was being used and not any other parametrized type.

It took me way to long to realize all of this 😭

cc @jchavarri I'm not sure about a few things here:

• What's ({txt= Ldot (Lident "*predef*", "option"); _}, [type_])
• Maybe that's needed for some case? If that's true I might be able to construct the same AST, but again, unsure why was there in the first place.

[davesnx]

Looks like tests are failing, and I believe there's some lies in my assumptions. *predef* looks like is used to wrap the inner string. Can you provide a test-case for this one @jchavarri?

[jchavarri]

@davesnx this part comes mostly from the adapted version of reason-react ppx that I used originally.

Looking at the code:

https://github.com/rescript-lang/syntax/blob/master/src/reactjs_jsx_ppx_v3.ml#L181

There was a when clause at the end of the 3 patterns of this branch. When I refactored the ppx to remove the isOptional function (as we could just use pattern matching Optional _ as label) I missed that I should apply it to the 2 patterns in lines 376-386 that the PR is currently removing.

I think the fix should leave the two patterns, but reapply the missing Optional, so something as follows:

        (* ~foo: option(int)=? *)
        | ((Optional _label)
          , Some {ptyp_desc= Ptyp_constr ({txt= Lident "option"; _}, [type_]); _}
          , _ )
        | ((Optional _label)
          , Some
              { ptyp_desc=
                  Ptyp_constr
                    ({txt= Ldot (Lident "*predef*", "option"); _}, [type_])
              ; _ }
          , _ )

The reason for this behavior is that optional labelled arguments are implemented as option types, so in this particular case we need to extract the internal value. The regression caused the pattern to match any kind of argument, be it optional, labelled, or normal.

It would be great to include a test to check everything's fine, we can use the component in https://github.com/ml-in-barcelona/jsoo-react/issues/85, plus another example where the param is really optional (with the =?).

What's ({txt= Ldot (Lident "*predef*", "option"); _}, [type_])

I am not sure tbh. Old versions of reason-react ppx used to create idents with *predef* in it: https://github.com/rescript-lang/rescript-compiler/blob/071bad06b4ef6664645b312ef3cdc648d311b815/lib/4.02.3/reactjs_jsx_ppx_v2.ml#L61. But latest versions don't. I think we can just remove that case, but I'm not 100% sure.

[jchavarri]

@davesnx merging this, thanks!