briskml / brisk-reconciler

React.js-like reconciler implemented in OCaml/Reason
MIT License
132 stars 17 forks source link

Implement JSX PPX #6

Closed wokalski closed 5 years ago

wokalski commented 5 years ago

Currently, there's additional boilerplate involved when using JSX; users have to manually implement createElement which is the function call that JSX is mapped to in Reason. Such function call is decorated with the [@JSX] attribute which allows any library to implement custom logic for the transformation.

We'd like to leverage it to remove the boilerplate. The specific implementation of this preprocessor will depend on the component creation API. A potential lightweight first class module based API like this is possible.

Another important question is if we want to use mainly upper case or lower case JSX. I am in favour of keeping the JSX as close to component definitions as possible. I.e. if we end up with the API above, I think we should have the lower case JSX. If we have a functor api for instance, we should use upper case JSX.

bryphe commented 5 years ago

@jchavarri did an awesome prototype for reactify that showcases how much the 'boilerplate' could be simplified:

prototype

Re lower-case / upper-case JSX - it seems like the consensus on Discord was that it should be lower-case since the component is a 'function' and functions can only have lowercase identifiers in Reason/OCaml.

bryphe commented 5 years ago

The custom component syntax feels verbose, especially duplication between make and createElement - it'd be so nice to have this streamlined!

jchavarri commented 5 years ago

Thanks @bryphe! I can try to find it that code and put it in some branch if it could serve in any way to resolve this issue.

One note about the different options available. There are two ppx strategies to simplify the user experience:

  1. Component declaration ppx: Have a ppx that allows to declare components (like let%reve in the gif above). This would transform a plain function into the fully fleshed module.

  2. JSX ppx: Overwrite the default JSX ppx (what @wokalski is suggesting in this issue). This allows brisk-reconciler to gain control over what <Comp /> means, so it can be translated into a function that is closer to what the reconciler needs (i.e. add / remove params, change createElement naming for st else etc).

I think it's worth to explore 2 in depth before attempting 1. The reasoning is that 1 involves way too much magic imo, which could make things trickier down the line for debugging, as the whole component creation process would become quite opaque.

2 otoh doesn't add any new magic, the ppx is already there and expected. Having 2 in place might allow to simplify the component declaration enough to maybe not need an extra component declaration ppx.

jchavarri commented 5 years ago

From a convo in Discord yesterday but adding it here as well:

Now that @wokalski has updated the reconciler so components can be just functions in lower case 🎉 I was thinking how this affects the ppxs we were discussing above. I think the need for a ppx is largely reduced now. But the changes in ReasonReact change the scenario a bit. In particular, I'm very curious about the part that converts labelled args into records. I'm not sure what the reasoning behind that change is, maybe @rickyvetter and @cristianoc could share more info about it and how likely it is that this is the approach taken in the future? There are many upsides for the Brisk reconciler to stay as close to ReasonReact as possible. 🙂

wokalski commented 5 years ago

@jchavarri I think it's simply because the "new" Reason React functional component is a valid component in JS. No more wrapping/unwrapping! From what I see the props are converted to a js object - just like props object in React.js.

wokalski commented 5 years ago

Adding to this, I think we will need a PPX when we want to have fragment support! It's probably coming later this month though. For the next two weeks, I'm going to focus on bug fixes and Brisk UI lib.

cristianoc commented 5 years ago

@jchavarri as @wokalski says most of the PPX for the upcoming hooks ReasonReact is about translating labeled arguments to objects. That need disappears when the UI framework is written in the same language, and not in JS.

rickyvetter commented 5 years ago

Yeah the ppx handles keys, refs, and children differently which might be useful for you all as well. But yeah - a lot of the PPX will not be relevant and can safely noop.

jchavarri commented 5 years ago

@cristianoc @rickyvetter That makes a lot of sense, thanks!

@wokalski @bryphe It seems then that the main advantage of the ppx for brisk-reconciler would be to release users of the need to thread the hooks (ex-slots) value through the different functions. Or are there any other benefits?

wokalski commented 5 years ago

@jchavarri There are two ppxs in this discussion:

  1. JSX PPX
  2. Component PPX

While the latter might be very interesting in the context of statically typed hooks, I'd like to put the discussion about it in a separate issue. This issue is mainly about the first one. I think we need JSX ppx for fragments and for children. Lists and Fragments have different semantics in react and it makes sense not to confuse the two. We could statically enforce all elements in lists to have keys for instance while fragments would just use positional matching with optional keys. I'd like to focus on this part in this issue but it'd be great if we had a separate issue for the component ppx.

rickyvetter commented 5 years ago

I think the children part of the function component ppx is still relevant for this conversation. With the new ppx these two examples desugar to the same thing

<Foo> ...(child) </Foo>
<Foo> child </Foo>

And this example desugars to a fragment wrap instead of an array wrap:

<Foo> child1 child2 </Foo>

For composite components children are always passed as part of the props object (a regular prop argument) and never through the variadic form.

Conceptually only primitive components (divs, fragments, etc) have a concept of variadic/keyed children.

wokalski commented 5 years ago

Fixed in #42