Allow labelled arguments after () when defining components using %component syntax

[glennsl]

This allows partially applied components to be returned with JSX syntax. E.g.:

module PartiallyAppliedComponent = {
  let%component make = ((), ~arg: string, hooks) =>
    (<Text title=arg />, hooks);
};

module PartiallyAppliedComponentConsumer = {
  let%component make = ((), hooks) => {
    let comp = <PartiallyAppliedComponent />;
    (comp(~arg="foo"), hooks);
  }
}; 

[wokalski]

What happens if you partially apply a component now?

[glennsl]

What do you mean by "partially apply"? in this case? Mandatory props can't be left out because of the (), and the description shows an example of partially applying non-prop arguments.

[wokalski]

@glennsl this is currently valid:

let testComponent = (~prop, ~prop2, ~dispatch, ()) => prop + prop2 - dispatch;

let x: (~dispatch: int) => int = <testComponent prop=1 prop2=3 />;

[wokalski]

That said, I think the current restriction (edit: maybe) should be lifted. Why not allow all kinds of functions (with named arguments) as let%component? The trade off is the following:

- Bad error reporting and not beginner friendly component declarations
+ Allows for any kinds of DSLs while leveraging let%component
- Less flexibility? Since we expect users to use let%component and [@JSX] we have a lot of flexibility and can make a lot of significant changes without breaking changes.
- And generally we'd have to think about how to handle the hooks variable. A simple [@hook] attribute attached to an argument could be enough for the cases where people don't use let%hook

I'm also considering the future when there's Tyxml. I'm pretty sure OCamlers might really like the approach. Are they going to be put off by the restrictions? Is it really bad if you have to do:

div ~style:some_style () ~children:[
]

as opposed to

div ~style:some_style [
]

I'm not sure!

[glennsl]

this is currently valid

Ah, yes, sorry, of course it is. This PR doesn't change that. The generated code is exactly the same.

Bad error reporting and not beginner friendly component declarations

Just because it wouldn't say "expected labelled argument or ()", or are there other bad errors you could get?

Less flexibility? Since we expect users to use let%component and [@jsx] we have a lot of flexibility and can make a lot of significant changes without breaking changes.

What kind of breaking changes do you foresee? It's pretty much just the hooks implementation isn't it? Other than that it's pretty much just an ordinary function.

I would love to be able to use the last form. And probably wouldn't use the more verbose one, I'd just use Reason and JSX instead. But if the stability of the hooks implementation is the only likely breaking change, I think it ought to wait until that stabilizes.

[wokalski]

Ah, yes, sorry, of course it is. This PR doesn't change that. The generated code is exactly the same.

so why allow that ?

Just because it wouldn't say "expected labelled argument or ()", or are there other bad errors you could get?

When you use JSX - the transformation depends on the terminating (). It’s because children is an optional argument. We could replace it with something like `NoChildren and use row polymorphism but then we don’t have default values. So it’s not worth it I think.

I don’t foresee any in the near future but should they arrive with two ppxes it’s way more flexible.

When it comes to the PR, I’m not sure what’s the value proposition but it doesn’t introduce any problematic side effects as far as I’m concerned (as long as optional args are not allowed). Aside from the PR, I still don’t have a clear idea of where we wanna be with the ppxes, API, and interoperability (between different component flavors so to speak).

[glennsl]

so why allow that ?

1. To be able to hide the extra arguments behind an abstract type. (~prop, ~dispatch, ()) => element won't unify with (~prop:prop, unit) => ((~dispatch:dispatcher) => element)
2. To visually separate the props from the "extra" arguments intended to be partially applied. While not technically necessary, it's more clear about intention.

Also bear in mind that I started on this thinking I could add positional arguments after () and to distinguish hooks as just being the last argument, but of course that's not always the case. So we need some other way to distinguish it, and making the extra arguments labelled is one way of doing that, but has the down-side of making it possible to apply it as a prop. Unless it's hidden from the end-user by an abstract type, which I think it could be.

I had two other suggestions for distinguishing them:

1. unlabelled arguments before ():
   (~prop, dispatch, (), hooks)

2. An extension point in place of the hooks argument:
   (~prop, (), dispatch, [%hooks])

The first would involve reordering the function arguments, which isn't great, and the second would involve adding another extension point with associated pile of code.

I suppose we could also add an attribute to tag the extra arguments with, but that kind of complexity doesn't seem anywhere near worth it:
(~prop, (), [@notHooks] dispatch, hooks)

When you use JSX - the transformation depends on the terminating (). It’s because children is an optional argument. We could replace it with something like `NoChildren and use row polymorphism but then we don’t have default values. So it’s not worth it I think.

Right, I was thinking children would always be a list and could be used in place of (), but of course we want to pass in other types of values as well, like render functions.

When it comes to the PR, I’m not sure what’s the value proposition but it doesn’t introduce any problematic side effects as far as I’m concerned (as long as optional args are not allowed).

Optional arguments are currently allowed, but you're right, they definitely shouldn't be. I'll fix.

[glennsl]

Rebased, banned optional args after () and fixed a bug (introduced by me) that silently ignored all errors by removing the use of Ast_pattern.parse since it can't distinguish between not finding a pattern and any other kind of error.

[wokalski]

I am starting to think; wouldn't it be better if you had a separate PPX for the elm like thing?

[glennsl]

As a separate project or separate extension point?

This is the only change I need, I'm pretty sure, and I don't see it making sense just for this. But to better support OCaml/Elm-like syntax it might make sense. That'd ne nice to have, but it's far from a requirement for what I'm trying to do.