JSX V4 WIP #517

mununki · 2022-06-02T01:20:18Z

Hi, team!
I'd like to convey my sincere appreciation for your hard work and contributions as a big fan of ReScript.

This PR adds the syntax support for JSX spread props such as below. I'm not sure whether the contribution from others besides the core team is open or not. But I think this feature will help many jsx use cases easier and cleaner. I didn't write the test yet, but please let me know and have a guide to writing a test for this case.

module Foo = {
  let bar = {
    name: "ReScript",
    attr: "Awesome"
  }
}

let baz = {
  nameFromBaz: "ReScript",
  attrFromBaz: "Awesome"
}

@react.component
let make = () => {
  <>
    <div id="1" onClick={_ => ()} {...Foo.bar}> {`div`->React.string} </div>
    <Foo id="2" onClick={_ => ()} {...baz}> {`Foo`->React.string} </Foo>
  </>
}

It is parsed and extended to

// ./lib/rescript.exe -ppx jsx test.res
module Foo = {
  let bar = {
    name: "ReScript",
    attr: "Awesome",
  }
}

let baz = {
  nameFromBaz: "ReScript",
  attrFromBaz: "Awesome",
}
@obj external makeProps: (~key: string=?, unit) => {.} = ""

let make = () => {
  ReactDOMRe.createElement(
    ReasonReact.fragment,
    [
      ReactDOMRe.createDOMElementVariadic(
        "div",
        ~props=ReactDOMRe.domProps(
          ~id="1",
          ~onClick={_ => ()},
          ~name="ReScript",
          ~attr="Awesome",
          (),
        ),
        [{`div`->React.string}],
      ),
      React.createElement(
        Foo.make,
        Foo.makeProps(
          ~id="2",
          ~onClick={_ => ()},
          ~nameFromBaz="ReScript",
          ~attrFromBaz="Awesome",
          ~children={`Foo`->React.string},
          (),
        ),
      ),
    ],
  )
}
let make = {
  let \"Test" = (\"Props": {.}) => make()
  \"Test"
}

cristianoc · 2022-06-02T01:47:41Z

Hey @mattdamon108 normally Foo would be defined in another file, that one has no access to in a ppx.

cristianoc · 2022-06-02T01:49:00Z

In general, would you ask for feedback in the forum, as chances there are people who have thought about this already.

mununki · 2022-06-02T02:00:54Z

normally Foo would be defined in another file, that one has no access to in a ppx.

@cristianoc Yes, it is. The ppx has no access to it either. But I'd like to add the functionality to access the module which exists in the same file.

normally Foo would be defined in another file, that one has no access to in a ppx.

Sure thing, thanks.

JsonKim · 2022-06-02T09:06:48Z

Thank you for your work. I have something to suggest.

Is it possible to output the following when there is an input like the following?

input:

<input type=text {...register("name")} />

output:

React.cloneElement(
  ReactDOMRe.createDOMElementVariadic("input", ~props=ReactDOMRe.domProps(~type_="text", ()), []),
  register("name"),
)

What I would suggest is to use the expression as-is. This means automating the use of cloneElement. Or does anyone have a better idea?

mununki · 2022-06-04T03:51:29Z

10d7dec rebase and force-pushed due to pulling the recent commits from master. Build with dune 🥇👍

cristianoc · 2022-06-04T04:16:42Z

@mattdamon108 to continue the discussion about implementation, this looks like a feature that needs type (hence scope) information.
Untyped parse trees do not provide type information, so if one would like to go ahead, some different route would need to be explored.

Not sure if this is something you would like to explore. Likely it will need to be explored during a longer time frame.

cristianoc · 2022-06-04T04:37:29Z

Also it might be useful to focus on examples that cannot be equivalently done with a function.

type pr = {
  sizes: string,
  srcDoc: string,
}

let baz = {
  sizes: "ReScript",
  srcDoc: "Awesome",
}

@react.component
let make = () => {
  let mkDiv = (~id, ~props as {sizes, srcDoc}, ~children) =>
    <div id onClick={_ => ()} sizes srcDoc> children </div>
  <>
    {mkDiv(~id="1", ~props=baz, ~children=React.string("ab"))}
    {mkDiv(~id="2", ~props=baz, ~children=React.string("cd"))}
  </>
}

cristianoc · 2022-06-04T04:40:23Z

or as

@react.component
let make = () => {
  module Div = {
    @react.component
    let make = (~id, ~props as {sizes, srcDoc}, ~children) =>
      <div id onClick={_ => ()} sizes srcDoc> children </div>
  }
  <>
    <Div id="1" props=baz> {"ab"->React.string} </Div>
    <Div id="2" props=baz> {"cd"->React.string} </Div>
  </>
}

The language offers already a lot of abbreviation mechanisms, so it might be good to explore what are examples that cannot be expressed this way.

mununki · 2022-06-04T05:38:06Z

@cristianoc That would be alternatives at this moment without syntax support and instead of this PR. But actually, the point is spreading by language, not by the user. For example,

@react.component
let make = () => {
  let { register } = ReactHookForm.use() // register: string => {onChange, onBlur, ref, ... }

  <input id="1" {...register("name")} />
}

I know that this PR can't solve this use case but, my ultimate intention is to solve this use case with syntax support. I think this feature will embrace more Js users and helps a lot to interop the Js modules.

I agree that this can't be resolved with an untyped parse tree. I already encountered several times during making the ppxes. I want to explore the journey to resolve this syntax support if the core team has willing to adopt this language feature in future.

Anyway, can you give a clue on how to access the expression or type after making typedtree? How to traverse the all merged typedtrees from one to another? I'm reading the reanalyze to search for a clue, but no gain so far. I'm still learning and not sure what actually the flambda and lambda IR is, but is it possible to spread in that step?

cristianoc · 2022-06-04T06:42:14Z

@cristianoc That would be alternatives at this moment without syntax support and instead of this PR. But actually, the point is spreading by language, not by the user. For example,
@react.component
let make = () => {
  let { register } = ReactHookForm.use() // register: string => {onChange, onBlur, ref, ... }

  <input id="1" {...register("name")} />
}
I know that this PR can't solve this use case but, my ultimate intention is to solve this use case with syntax support. I think this feature will embrace more Js users and helps a lot to interop the Js modules.

I agree that this can't be resolved with an untyped parse tree. I already encountered several times during making the ppxes. I want to explore the journey to resolve this syntax support if the core team has willing to adopt this language feature in future.

Anyway, can you give a clue on how to access the expression or type after making typedtree? How to traverse the all merged typedtrees from one to another? I'm reading the reanalyze to search for a clue, but no gain so far. I'm still learning and not sure what actually the flambda and lambda IR is, but is it possible to spread in that step?

In this example, what's the type of register("name")?
In other words, is this untyped or typed spread?

As of possible ways forward, it seems likely that advancing this #521 would be a stepping stone towards this.

mununki · 2022-06-04T06:54:31Z

In my example, the type of register("name") is type t.

type t = {
  onChange: ReactEvent.Form.t => unit,
  onBlur: ReactEvent.Focus.t => unit,
  ref: ReactDOM.domRef,
  name: string,
}

let register: string => t

cristianoc · 2022-06-04T07:00:13Z

OK then I think this feature will come for free with #521
So the way to process on this feature is to progress on that.
And the firs step to make that happen is to help with rescript-lang/rescript#5412

mununki · 2022-06-04T08:32:41Z

I got your point. If the jsx is parsed to the normal function and passes the object as arguments based on the more general structural typing (case no. 1), I agree that it is the first step to advance to make happen. But I'm still unclear about case no. 2, how to make structurally typing this multiple arguments layer?

@react.component
let make = () => {
  let { register } = ReactHookForm.use()

  // case 1. OK
  // just parse it to ReactDOMRe.createDOMElementVariadic("input", ~props=register("name"))
   <input {...register("name")} />

  // case 2. unclear
  // {"id": string, "type_": string} and {"onChange": ... , "onBlur": ... , "ref": ..., "name": string }
  <input id="1" type_="button" {...register("name")} />
}

cristianoc · 2022-06-04T08:56:33Z

That only requires record spread, which is part of the language.
{...foo, x:e}.

mununki · 2022-06-04T09:07:01Z

Wow, I've verified this just moment ago.

type foo = {name: string}

let foo = {
  name: "woonki",
}

let foo1 = foo

type bar = {
  name: string,
  age: int,
}

let bar = {...foo1, age: 10}

// let bar = {...foo1, age: 10}
structure_item (test.res[9,76+0]..[12,117+1])
    Pstr_type Nonrec
    [
      type_declaration "bar" (test.res[9,76+5]..[9,76+8]) (test.res[9,76+0]..[12,117+1])
        ptype_params =
          []
        ptype_cstrs =
          []
        ptype_kind =
          Ptype_record
            [
              (test.res[10,89+2]..[10,89+14])
                Immutable
                "name" (test.res[10,89+2]..[10,89+6])                core_type (test.res[10,89+8]..[10,89+14])
                  Ptyp_constr "string" (test.res[10,89+8]..[10,89+14])
                  []
              (test.res[11,105+2]..[11,105+10])
                Immutable
                "age" (test.res[11,105+2]..[11,105+5])                core_type (test.res[11,105+7]..[11,105+10])
                  Ptyp_constr "int" (test.res[11,105+7]..[11,105+10])
                  []
            ]
        ptype_private = Public
        ptype_manifest =
          None
    ]

mununki · 2022-06-04T09:16:43Z

I got your point again. the more general structural typing and the immutable updated record will solve cases 1, 2! I think it needs to change the jsx mapper either. Can I ask how the progress is going? I think I can contribute to making up for this PR further.

cristianoc · 2022-06-04T09:18:40Z

There's no work except for the little text written in the issues.
So any help there is useful to make progress.

mununki · 2022-06-04T09:20:16Z

Got it with thanks! I'm gonna work on this PR and let you guys know.

cristianoc · 2022-06-04T09:20:26Z

First, test the new record types' functionality.
I've already reported an issue with error messages. There might be more.

Solving the error message issue is also useful.

mununki · 2022-06-04T09:20:59Z

Solving the error message issue is also useful.

Checked, good to know that!

mununki · 2022-06-06T13:07:25Z

@cristianoc I've implemented the spread props feature with the general type checking of the structural typings. https://forum.rescript-lang.org/t/rfc-more-general-type-checking-for-structural-typings/1485/73 This is quite a visible change behind the jsx definition. This PR will change how to define the makeProps and make for jsx parsing and mapping.

// as-is
@obj external makeProps: (~key: string=?, unit) => {.} = ""
let make = () => {
  ReactDOMRe.createElement(
    ReasonReact.fragment,
    [
      React.createElement(
        Foo.make,
        Foo.makeProps(~id="2", ~onClick={_ => ()}, ~children={`Foo`->React.string}, ()),
      ),
    ],
  )
}

// to-be
@obj                   
type makeProps = {
  key: option<string>,
}
let makeProps = (props: makeProps) => props
let make = () => {
  ReactDOMRe.createElement(
    ReasonReact.fragment,
    [
      React.createElement(
        Foo.make,
        Foo.makeProps({id: "2", onClick: _ => (), children: {`Foo`->React.string}}),
      ),
    ],
  )
}

I well acknowledge that this RFC needs more tests and polishing rescript-lang/rescript#5412, but I hope this PR could be one of the good samples and tests for the RFC as well.

mununki · 2022-06-06T13:14:20Z

spread props

module Foo = {         
  @obj
  type makeProps<'id, 'onClick, 'onBlur, 'children> = {
    key: option<string>,
    id: 'id,
    onClick: 'onClick,
    onBlur: 'onBlur,
    children: 'children,
  }
  let makeProps = (props: makeProps) => props
  let make =
    (
      @warning("-16") ~id,
      @warning("-16") ~onClick,
      @warning("-16") ~onBlur,
      @warning("-16") ~children,
    ) => {
      ReactDOMRe.createDOMElementVariadic("div", [{`Foo`->React.string}])
    }
  }

@obj
type makeProps = {
  key: option<string>,
}
let makeProps = (props: makeProps) => props

let make = () => {
  ReactDOMRe.createElement(
    ReasonReact.fragment,
    [
      ReactDOMRe.createDOMElementVariadic(
        "div",
        ~props={...foo, id: "1", onClick: _ => ()},
        [{`div`->React.string}],
      ),
      React.createElement(
        Foo.make,
        Foo.makeProps({...foo, id: "2", onClick: _ => (), children: {`Foo`->React.string}}),
      ),
    ],
  )
}

cristianoc · 2022-06-06T14:25:28Z

@mattdamon108 how about adding the corresponding pr in the compiler repo, which uses this as syntax, and shows a few compiled examples with their generated code.

One of the questions is whether makeProps is still needed. Can one do without?

cristianoc · 2022-06-06T14:29:27Z

Also it might be best to start with simple examples, rather than the new spread, which is a more advanced case.

Also, this should probably be called ppx version 4.

For the purpose of gradual upgrade the two need to at least co-exist.

In addition, one should see if that is enough for a v4 project using v3 dependencies.

mununki · 2022-06-06T14:35:31Z

how about adding the corresponding pr in the compiler repo, which uses this as syntax, and shows a few compiled examples with their generated code.
Also it might be best to start with simple examples, rather than the new spread, which is a more advanced case.

Agree. ~~Do you want me to make PR in the compiler repo without the new spread first? It isn't hard to remove that part.~~ You mean the example without spread case, I got it.

One of the questions is whether makeProps is still needed. Can one do without?

I had the same question, but I think it is still needed because it has a role to check the type of props in the record, unless we are designing the make function has the argument as record without makeProps. But I'm not sure of it. I think we better keep the make fn using the labelled arguments per each prop. What do you think?

mununki · 2022-06-06T14:42:07Z

And plus, react.createElement API interface looks like. https://reactjs.org/docs/react-api.html#createelement 🤷‍♂️

React.createElement(
  type,
  [props],
  [...children]
)

cristianoc · 2022-06-06T15:03:48Z

Yes, the idea is to illustrate the new mechanism with simpler examples first, then the prop spread comes later (in the description), as an advanced example. The implementation can cover all at once, does not matter.

Notice the previous work on this: #235
Haven't reviewed either one in detail. But it would be good to have an understanding of what has been tried and current thoughts in a little summary.

Following https://forum.rescript-lang.org/t/jsx-v4-next-rescript-react-0-10-x/3431/10

only show how to transform the arrow function.

mununki · 2022-06-14T15:12:39Z

I missed adding test files, then I found that some tests failed after pushing the commits. 1c185ed I'll fix it.
Several thoughts came to mind while I'm implementing the applying @react.component to fundef. Replacing @react.component from Pstr_value to Pexp_fun is not enough to generate the output. e.g. type props is generated in place of Pstr_value, not Pexp_fun. So I have to generate type props and another structure_item to shadow the make function to capitalized name, then I add @react.component attr to Pexp_fun. It seems more abuse of ppx to me.

mununki · 2022-06-14T15:19:32Z

I have an idea to generate the proper output for forwardRef case without replacing @react.component. 1c185ed commit is just FYI to peek. I'm willing to reset the commits and implement it again. What do you think?

cristianoc · 2022-06-14T15:22:17Z

It does not need to literally change the ast. It can be just a guide on how to structure the implementation.

cristianoc · 2022-06-14T15:25:41Z

The spec indicates what is the result of the transformation, it does not impose a specific way to implement it.
Declarative specification.

mununki · 2022-06-14T15:27:54Z

Oh I misunderstood your intention. I thought the changing ast will be more cleaner desgin, but I found it is not. 🥲

mununki · 2022-06-14T15:28:34Z

I'll reset the latest 2 commits, then I'll fix the forwardRef example. I think I can make it better.

cristianoc · 2022-06-14T23:10:40Z

I have renamed this PR.
Also, perhaps this is a good time to start a new one, as scrolling through this page takes a while.

mununki · 2022-06-14T23:12:16Z

OK I'll make a new PR

mununki · 2022-06-15T07:34:45Z

Closed. Further discussion in #547

mununki changed the title ~~Add parsing and jsx ppx for spread props~~ Add parsing and jsx ppx support for spread props Jun 2, 2022

mununki force-pushed the spread-props branch from 32ee826 to 10d7dec Compare June 4, 2022 03:38

mununki force-pushed the spread-props branch from 10d7dec to abb5189 Compare June 6, 2022 12:52

mununki and others added 15 commits June 13, 2022 23:37

support React.forwardRef

9738739

replace filterMap with std lib

ded39d4

Add JSX V4 spec.

14f4a8b

typo

cd0989c

JSX spec: move createElement to application.

8176e6b

Following https://forum.rescript-lang.org/t/jsx-v4-next-rescript-react-0-10-x/3431/10

spec: comment on name

674dd43

spec: clean up file

ee98a9f

external

e9d9eea

Be consistent with interface

25a531a

only show how to transform the arrow function.

Update JSXV4.md

cb0664f

restore React.createElement in application site

3434c72

spec: react fragment

09a9164

spec: tweak fragment.

905f19c

make react component name capitalized

2995a6d

test: add test files

d15b125

mununki force-pushed the spread-props branch from d88a2a4 to d15b125 Compare June 13, 2022 14:37

cristianoc changed the title ~~Add parsing and jsx ppx support for spread props~~ JSX V4 WIP Jun 14, 2022

fix ppx for forwardRef

0bbb5f8

mununki force-pushed the spread-props branch from d85ef30 to 0bbb5f8 Compare June 15, 2022 07:17

mununki closed this Jun 15, 2022

mununki deleted the spread-props branch June 15, 2022 07:34

JSX V4 WIP #517

JSX V4 WIP #517

Conversation

mununki commented Jun 2, 2022 • edited by cristianoc Loading

cristianoc commented Jun 2, 2022

cristianoc commented Jun 2, 2022

mununki commented Jun 2, 2022 • edited Loading

JsonKim commented Jun 2, 2022

mununki commented Jun 4, 2022 • edited Loading

cristianoc commented Jun 4, 2022

cristianoc commented Jun 4, 2022

cristianoc commented Jun 4, 2022

mununki commented Jun 4, 2022 • edited Loading

cristianoc commented Jun 4, 2022

mununki commented Jun 4, 2022

cristianoc commented Jun 4, 2022

mununki commented Jun 4, 2022 • edited Loading

cristianoc commented Jun 4, 2022 • edited Loading

mununki commented Jun 4, 2022

mununki commented Jun 4, 2022

cristianoc commented Jun 4, 2022

mununki commented Jun 4, 2022

cristianoc commented Jun 4, 2022

mununki commented Jun 4, 2022

mununki commented Jun 6, 2022 • edited Loading

mununki commented Jun 6, 2022

cristianoc commented Jun 6, 2022

cristianoc commented Jun 6, 2022

mununki commented Jun 6, 2022 • edited Loading

mununki commented Jun 6, 2022

cristianoc commented Jun 6, 2022 • edited Loading

mununki commented Jun 14, 2022 • edited Loading

mununki commented Jun 14, 2022 • edited Loading

cristianoc commented Jun 14, 2022

cristianoc commented Jun 14, 2022

mununki commented Jun 14, 2022

mununki commented Jun 14, 2022

cristianoc commented Jun 14, 2022

mununki commented Jun 14, 2022

mununki commented Jun 15, 2022

mununki commented Jun 2, 2022 •

edited by cristianoc

Loading

mununki commented Jun 2, 2022 •

edited

Loading

mununki commented Jun 4, 2022 •

edited

Loading

mununki commented Jun 4, 2022 •

edited

Loading

mununki commented Jun 4, 2022 •

edited

Loading

cristianoc commented Jun 4, 2022 •

edited

Loading

mununki commented Jun 6, 2022 •

edited

Loading

mununki commented Jun 6, 2022 •

edited

Loading

cristianoc commented Jun 6, 2022 •

edited

Loading

mununki commented Jun 14, 2022 •

edited

Loading

mununki commented Jun 14, 2022 •

edited

Loading