ml-in-barcelona / jsoo-react

js_of_ocaml bindings for ReactJS. Based on ReasonReact.
https://ml-in-barcelona.github.io/jsoo-react
MIT License
138 stars 19 forks source link

OCaml syntax proposal: unlabeled children #105

Closed glennsl closed 2 years ago

glennsl commented 2 years ago

Spoke briefly about this with @jchavarri yesterday.

Currently, both dom elements (lower-case components) and custom (upper-case) components require children to be passed through an argument labeled ~children, and to have a terminating unit argument since there's no other positional arguments. From a consumer point of view this seems unnecessary when children as an unlabeled argument could also serve the role as terminator.

Pros:

Cons:

Example

Currently, with labeled children:

let%component make () =
  React.Fragment.make
    ~children: [
      a ~href:"https://github.com/jihchi/jsoo-react-realworld-example-app" ~target:"_blank" 
        ~children: [ 
          i ~className:"ion-social-github" ~children:[] (); 
          React.string "Fork on GitHub";
        ]
        ();
      footer
        ~children: [
          div ~className:"container"
            ~children: [
              Link.make ~onClick:(Link.location Link.home) ~className:"logo-font"
                ~children:[ React.string "conduit" ]
                ();
              span ~className:"attribution"
                ~children: [
                  React.string "An interactive learning project from ";
                  a ~href:"https://thinkster.io" ~children:[ React.string "Thinkster" ] ();
                  React.string ". Code & design licensed under MIT.";
                ]
                ();
            ]
            ();
        ]
        ();
    ]
    ()

could become

let%component make () =
  React.Fragment.make [
    a ~href:"https://github.com/jihchi/jsoo-react-realworld-example-app" ~target:"_blank" [
      i ~className:"ion-social-github" [];
      React.string "Fork on GitHub";
    ];
    footer [
      div ~className:"container" [
        Link.make ~onClick:(Link.location Link.home) ~className:"logo-font" [
          React.string "conduit"
        ];
        span ~className:"attribution" [
          React.string "An interactive learning project from ";
          a ~href:"https://thinkster.io" [ React.string "Thinkster" ];
          React.string ". Code & design licensed under MIT.";
        ];
      ];
    ];
  ]

or alternatively, if you prefer (same syntax, just different formatting)

let%component make () =
  React.Fragment.make
    [ a ~href:"https://github.com/jihchi/jsoo-react-realworld-example-app" ~target:"_blank"
        [ i ~className:"ion-social-github" []
        ; React.string "Fork on GitHub"
        ]
    ; footer 
        [ div ~className:"container" 
            [ Link.make ~onClick:(Link.location Link.home) ~className:"logo-font" 
                [ React.string "conduit" ]
            ; span ~className:"attribution" 
                [ React.string "An interactive learning project from "
                ; a ~href:"https://thinkster.io" [ React.string "Thinkster" ]
                ; React.string ". Code & design licensed under MIT."
                ] 
            ]
        ]
    ]
jchavarri commented 2 years ago

Thanks @glennsl for the proposal!

One immediate downside I can think of is that migrating from Reason to OCaml syntax becomes a bit harder. Right now one can just run existing Reason code through refmt, remove some extra attributes needed by Reason syntax and that's about it. But I guess it should not be a very big deal 🤔 . Or we could always leave a version up to which labelled children is supported to help with migrations of this kind.

My main concern is about uppercase components, where unlike lowercase components, children prop is optional. Does the proposal assume consumers of an uppercase component that ignores children would need to pass an empty list as children to all elements created by that component? Or would ppx allow the last argument to be either unit or a list of elements?

One minor implementation concern is that Reason syntax originally translates <Foo /> into Foo.createElement ~children:[] ())[@JSX ], so the jsoo-react ppx would need to do some processing to transform it into the proposed solution. But this is not a big deal (we do the same already to transform from createElement to make).

If we agree to apply this change, I'd probably do it before v1. Or if we don't, release a v0.1 first, to make it clear that there might be breaking changes down the line.

glennsl commented 2 years ago

One immediate downside I can think of is that migrating from Reason to OCaml syntax becomes a bit harder.

Good point! Noted as a con in the proposal.

Does the proposal assume consumers of an uppercase component that ignores children would need to pass an empty list as children to all elements created by that component? Or would ppx allow the last argument to be either unit or a list of elements?

I was thinking it would pass () if children are omitted. Since children can have any type (e.g. <MyComponent> ...{() => ...} </MyComponent>), that seems more natural to me than []. It also means you don't have to do anything to migrate components without children.

Migration examples:

(* before, with children *)
let make ~foo ~children () = ... [@react.component]
(* after, with children *)
let%component make ~foo children = ...

(* before, without children *)
let make ~foo () = ... [@react.component]
(* after, without children *)
let%component make ~foo () = ...

... so the jsoo-react ppx would need to do some processing to transform it into the proposed solution.

Noted as well.

If we agree to apply this change, I'd probably do it before v1. Or if we don't, release a v0.1 first, to make it clear that there might be breaking changes down the line.

I can have a go at a PR for this when I get some time, next weekend perhaps, unless you want to give it a go yourself.