Feedback from the forum for JSX v4 #5646

mununki · 2022-09-07T16:05:10Z

cristianoc · 2022-09-08T07:09:42Z

For the type info in the editor, let's first see if adjusting the location of the props type definitio helps.

cristianoc · 2022-09-08T19:28:06Z

@mattdamon108 found out this by chance, a little thing that you might find interesting.

This innocent-looking code does not compile with V3:

module type OptT = {
  module type Opt2 = {
    @react.component
    let make: (~s: string) => React.element
  }
}

module Opt: OptT = {
  module Opt2 = {
    @react.component
    let make = (~s) => React.string(s)
  }
  module type Opt2 = module type of Opt2
}

The reason is that the type for makeProps in the implementation of Opt2 is polymorphic because there's no type annotation on the argument s.

So it's equivalent to this code:

module type OptT = {
  module type Opt2 = {
    @obj external makeProps: (~s: string, ~key: string=?, unit) => {"s": string} = ""
  }
}

module Opt: OptT = {
  module type Opt2 = {
    @obj external makeProps: (~s: 's, ~key: string=?, unit) => {"s": 's} = ""
  }
}
// Values do not match:
//    external makeProps: (~s: string, ~key: string=?, unit) => {"s": string} =
//  "" "#rescript-external"
//  is not included in
//    external makeProps: (~s: 's, ~key: string=?, unit) => {"s": 's} =
//  "" "#rescript-external"

However it compilers perfectly fine with V4 because there's no makeProps generated.

mununki · 2022-09-10T07:13:47Z

However it compilers perfectly fine with V4 because there's no makeProps generated.

It looks really cool. It is due to making the props type as polymorphic and using the type information(string) in the type declaration make. 👍

module type OptT = {                   
  module type Opt2 = {
    type props<'s> = {key?: string, s: 's}

    let make: React.componentLike<props<string>, React.element> // <- using string type information
  }
}

module Opt: OptT = {
  module Opt2 = {
    type props<'s> = {key?: string, s: 's}

    @react.component let make = ({s, _}: props<'s>) => React.string(s)
    let make = {
      let \"Interesting$Opt$Opt2" = (props: props<_>) => make(props)
      \"Interesting$Opt$Opt2"
    }
  }
  module type Opt2 = module type of Opt2
}

cristianoc · 2022-09-10T07:18:21Z

However it compilers perfectly fine with V4 because there's no makeProps generated.

It looks really cool. It is due to making the props type as polymorphic and using the type information(string) in the type declaration make. 👍

module type OptT = {                   
  module type Opt2 = {
    type props<'s> = {key?: string, s: 's}

    let make: React.componentLike<props<string>, React.element> // <- using string type information
  }
}

module Opt: OptT = {
  module Opt2 = {
    type props<'s> = {key?: string, s: 's}

    @react.component let make = ({s, _}: props<'s>) => React.string(s)
    let make = {
      let \"Interesting$Opt$Opt2" = (props: props<_>) => make(props)
      \"Interesting$Opt$Opt2"
    }
  }
  module type Opt2 = module type of Opt2
}

Yes that's precisely why it works.
Abstracting a bit form the details, it's because there's less magic, so no leaky abstraction.

cristianoc · 2022-09-12T06:48:36Z

@mattdamon108 wondering whether we should revisit key.

Ovservation: is it OK to pass key to a component that does not expect it? For normal props, that's an error.
I think it should be down to the component to define key (and JSX checks that it's of optional string type).
What do you think?

Separately there's the question of what happens when we pass no props (currently key:?None is used to encode an empty record), but we don't need to confuse the 2 different issues.

mununki · 2022-09-12T07:41:51Z

Thank you for raising the key issue.

I think this example shows what we need to fix. We don't need to pass the key in the props record. I believe we don't have the same issue in V4 + automatic. I've already removed the key not to pass to the component. Sorry for making confusion, key should be there React.createElement(Bar.make, {key: "k", x: "x"}).

// original
module Foo = {
  @react.component
  let make = () => <Bar key="k" x="x" />
}

// V4 + classic
module Foo = {
  type props = {key?: string}

  @react.component let make = (_: props) => React.createElement(Bar.make, {key: "k", x: "x"})
                                                                          ^^^^^^^^^^^^^^^^^^^
                                                                          // should be {key: "k", x: "x"}
  let make = {
    let \"Interesting$Foo" = props => make(props)
    \"Interesting$Foo"
  }
}

// V4 + automatic
module Foo = {                         
  type props = {key?: string}
  @react.component let make = (_: props) => React.jsxKeyed(Bar.make, {x: "x"}, "k")
  let make = {
    let \"Interesting$Foo" = props => make(props)
    \"Interesting$Foo"
  }
}

In the case of no props, I think we have to pass {key: ?None} to encode an empty record {}.

module Foo = {                         
  type props = {key?: string}
  @react.component let make = (_: props) => React.createElement(Bar.make, {key: ?None})
  let make = {
    let \"Interesting$Foo" = props => make(props)
    \"Interesting$Foo"
  }
}

What do you think?

mununki · 2022-09-12T08:19:59Z

is it OK to pass key to a component that does not expect it? For normal props, that's an error.
I think it should be down to the component to define key (and JSX checks that it's of optional string type).
What do you think?

Sorry for making confusion. Can you explain little more detail about it?

Generally we don't pass the key to the component, unless key props is populated.

// original
module Bar = {
  @react.component
  let make = (~x) => React.string(x)
}

module Foo = {
  @react.component
  let make = () => <Bar x="x" />
}

// converted
module Bar = {                         
  type props<'x> = {key?: string, x: 'x}
  ...
}

module Foo = {
  ...
  @react.component let make = (_: props) => React.createElement(Bar.make, {x: "x"}) // here
  ...
}

cristianoc · 2022-09-12T08:41:20Z

My proposal is to explore using key as follows:

@react.componet
let make = (~key=?, ~a) => <div ?key> {React.string(a)} </div>

If your component wants to use key then it needs to list it in the props.
If it does not, then you cannot pass key to it.

cristianoc · 2022-09-12T08:48:57Z

In the case of no props, I think we have to pass {key: ?None} to encode an empty record {}.

#5658

mununki · 2022-09-12T08:51:25Z

My proposal is to explore using key as follows:
@react.componet
let make = (~key=?, ~a) => <div ?key> {React.string(a)} </div>
If your component wants to use key then it needs to list it in the props. If it does not, then you cannot pass key to it.

I'm afraid it would violate the rule of React API, which is that key should not be used inside the component. This is an anti-pattern in React.js.

const Foo = ({key, x}) => {
  return (<div key> {x} </div>)
}

mununki · 2022-09-12T08:53:05Z

The prop key is special in Reac.js. No need to be defined, but all components have it.

cristianoc · 2022-09-12T08:57:41Z

The prop key is special in Reac.js. No need to be defined, but all components have it.

~~deleted confused comment~~

cristianoc · 2022-09-12T09:02:00Z

OK so since key should not be used inside the component, we should find a way to not make it appear in the definition of props.

cristianoc · 2022-09-12T09:03:58Z

I think the only reason why key is in the prop type is a little technicality:

React.createElement(Bar.make, {key: "k", x: "x"}

So we're passing the record to make, but we just said that make cannot use key as it violates the rules of React API.

cristianoc · 2022-09-12T09:05:12Z

So @mattdamon108 all what's needed it to add the key parameter to {x: "x"} without changing the type of {x: "x"}, correct? If we can do that, then we don't need key in the props type def. Right?

mununki · 2022-09-12T10:35:18Z

Ah.. I think I got your point and what you're solving. If I understand correctly, I think you're pointing 1 expression.

module Bar = {                         
  type props<'x> = {key?: string, x: 'x}
  ...
  let make = {
    let \"Interesting$Bar" = (props: props<_>) => make(props) // <-- 1
    \"Interesting$Bar"
  }
}

module Foo = {
  ...
  @react.component let make = (_: props) => React.createElement(Bar.make, {key: "k", x: "x"}) // <-- 2
  ...
}

(I've removed unnecessary lines.) React.createElement has a type.

@module("react")
external createElement: (component<'props>, 'props) => element = "createElement"

Actually, 1 doesn't actually call the make and pass the props including key in js runtime representation. 1 is the rescript expression to change the function name starting with a capital letter in the js expression. And the key field in the definition side is just for the type checking in the React.createElement. 2 should pass the record, which is transpiled to js object with key, then type checker can check the props type in Bar component. So, in my opinion, we don't need to get rid of the key from the definition side in the props type.

I hope this is right point to what you mean.

mununki · 2022-09-12T10:39:56Z

And all react components have the key prop basically, having a key field in the props type seems natural.

mununki · 2022-09-12T10:41:12Z

FYI, here is the js runtime representation after being transpiled.

function Interesting$Bar(props) {
  return props.x;
}

var Bar = {
  make: Interesting$Bar
};

function Interesting$Foo(props) {
  return React.createElement(Interesting$Bar, {
              key: "k",
              x: "x"
            });
}

var Foo = {
  make: Interesting$Foo
};

cristianoc · 2022-09-12T10:47:43Z

Let me try again. You need to pass key, as all components have it.
The only reason to have it in the type is a technicality.

You could have addKey : ('a, string) => 'a
use that and avoid putting key in props.

The issue is key in props is pure boilerplate that is user visible.

mununki · 2022-09-12T11:10:46Z

The only reason to have it in the type is a technicality.

You could have addKey : ('a, string) => 'a use that and avoid putting key in props.

The issue is key in props is pure boilerplate that is user visible.

Oh, it makes sense to me. But, still why do we need to get rid of key from the props?

cristianoc · 2022-09-12T11:32:27Z

It's because of the surprise effect and the boilerplate.

If I want to write a component directly, I need to define the key thing for every component.

Also, a component with 2 props has 3 fields. Can't think of first field and second field anymore.

When hovering, I see the key in the type.

When there's an error message, I see the key in the type.

When exporting the component to TS, genType needs to explicitly remove key from the props.

mununki · 2022-09-13T01:54:18Z

It's because of the surprise effect and the boilerplate.

If I want to write a component directly, I need to define the key thing for every component.

Also, a component with 2 props has 3 fields. Can't think of first field and second field anymore.

When hovering, I see the key in the type.

When there's an error message, I see the key in the type.

When exporting the component to TS, genType needs to explicitly remove key from the props.

It makes sense to me, now.
I've tried to implement it, but it seems very tricky. I tried with addKey: ('a, string) => 'a, but no gain. 'a should be a record type with key, but the type of our new props doesn't have a key.

On second try, I rewrite the addKey as a raw expression, but js output doesn't look good.

let addKey: ('a, string) => 'a = %raw(`
function(props, key) {
  return Object.assign(props, {"key": key})
}
`)

module Foo = {
  ...
  let make = (_: props) => React.createElement(Bar.make, addKey(({x: "x"}: Bar.props<string>), "k"))
  ...
}

mununki · 2022-09-13T02:02:16Z

I have another question about declaring the props type of the component which doesn't have any props. Is it possible to declare the empty record type by #5658 ?

type props = {}

cristianoc · 2022-09-13T03:42:39Z

It's because of the surprise effect and the boilerplate.
If I want to write a component directly, I need to define the key thing for every component.
Also, a component with 2 props has 3 fields. Can't think of first field and second field anymore.
When hovering, I see the key in the type.
When there's an error message, I see the key in the type.
When exporting the component to TS, genType needs to explicitly remove key from the props.

It makes sense to me, now. I've tried to implement it, but it seems very tricky. I tried with addKey: ('a, string) => 'a, but no gain. 'a should be a record type with key, but the type of our new props doesn't have a key.

On second try, I rewrite the addKey as a raw expression, but js output doesn't look good.
let addKey: ('a, string) => 'a = %raw(`
function(props, key) {
  return Object.assign(props, {"key": key})
}
`)

module Foo = {
  ...
  let make = (_: props) => React.createElement(Bar.make, addKey(({x: "x"}: Bar.props<string>), "k"))
  ...
}

One could use an inline function

@inline
let addKey = (o: 't, k): 't => Obj.magic(Js.Obj.assign(Obj.magic(o), {"key": k}))

or more likely a dedicated React.addElementWithKey.

cristianoc · 2022-09-13T03:44:05Z

I have another question about declaring the props type of the component which doesn't have any props. Is it possible to declare the empty record type by #5658 ?
type props = {}

This is a good question, and I don't have a good answer.
There currently is no support for empty record. One try to add it.
Or fall back to a dummy prop in that case.

cristianoc · 2022-09-13T03:45:27Z

I guess overall there's no way to pay zero price for key.
Currently, one pays the price constantly and uniformly.
The alternative is to pay the price of complexity only when key is used and when components have no props. Both cases in the minority of uses, but still they do happen.

cristianoc · 2022-09-13T03:53:06Z

I think using an empty record instead of the {key:?None} encoding is pretty clear.
The rest is less clear and should be explored a little more.

cristianoc · 2022-09-13T04:24:50Z

Actually the empty record type type empty = {} was already there in the syntax, but disabled.
Now added to the type checker too: #5658

mununki · 2022-09-13T13:19:15Z

Based on #5658 and addKey function, I think I can try to remove the key from the props. I’m wondering whether it is okay Object.assign in js representation with React.createElement api.

See #5646

cristianoc · 2022-09-18T09:16:53Z

Based on #5658 and addKey function, I think I can try to remove the key from the props. I’m wondering whether it is okay Object.assign in js representation with React.createElement api.

In master.

cristianoc · 2022-09-19T15:57:51Z

@mattdamon108 would you explain "Formatting {...str} to spreadProps=str"?
How is one supposed to use prop spread currently, and what is the issue?

mununki · 2022-09-19T16:11:55Z

Currently with JSX v4, the spread props is working only for uppercase.

module A = {
  @react.component
  let make = (~s, ~x) => (s ++ x)->React.string
}

@react.component
let make = () => {
  let str: A.props<_> = {
    s: "s",
    x: "x",
  }

  <A x="y" {...str} /> // <-- here
}

But, the formater is changing {...str} -> spreadProps=str

Screen.Recording.2022-09-20.at.1.09.40.AM.mov

If the formater keeps {...str} instead spreadProps=str, it would be more friendly to Js developers.

cristianoc · 2022-09-20T01:35:45Z

What if a component has a prop called spreadProps? Is this going to be a problem?

mununki · 2022-09-20T01:41:43Z

What if a component has a prop called spreadProps? Is this going to be a problem?

Yes, ppx assume that the labelled argument spreadProps is the props type of component in this case A component.

cristianoc · 2022-09-20T01:47:06Z

Looks like one needs to find some other way of encoding that information.
Can't remember how e.g. fragments <> are represented.

cristianoc · 2022-09-20T01:49:10Z

This is printing for fragments:

    | Pexp_construct _ when ParsetreeViewer.hasJsxAttribute e.pexp_attributes ->
      printJsxFragment ~customLayout e cmtTbl

cristianoc · 2022-09-20T01:50:31Z

And this is parsing:

(*
 * jsx-fragment ::=
 *  | <> </>
 *  | <> jsx-children </>
 *)
and parseJsxFragment p =
  let childrenStartPos = p.Parser.startPos in
  Scanner.setJsxMode p.scanner;
  Parser.expect GreaterThan p;
  let _spread, children = parseJsxChildren p in
  let childrenEndPos = p.Parser.startPos in
  Parser.expect LessThanSlash p;
  Parser.expect GreaterThan p;
  let loc = mkLoc childrenStartPos childrenEndPos in
  makeListExpression loc children None

cristianoc · 2022-09-20T01:51:39Z

So looks like fragments are represented as lists.
And to print, it recognises any construct (presumably for [] and :: for lists).

cristianoc · 2022-09-20T01:57:27Z

Looks like for example this is not taken:

    | Pexp_array [spread] when ParsetreeViewer.hasJsxAttribute e.pexp_attributes

cristianoc · 2022-09-20T01:59:14Z

So it could e.g. be encoded as an array of length 1 with a jsx attribute on it.
Both at the parsing and printing level.
Does it make sense?

cristianoc · 2022-09-20T01:59:46Z

Also, where is {...x} allowed to appear? I guess only once? Does it need to be at the end of other props?

mununki · 2022-09-20T02:08:19Z

Also, where is {...x} allowed to appear? I guess only once? Does it need to be at the end of other props?

Only 1 would be fine, I think.

So it could e.g. be encoded as an array of length 1 with a jsx attribute on it.
Both at the parsing and printing level.
Does it make sense?

If I understand correctly, parsing the {...str} as Pexp_array [str] instead of labelled argument, right?

cristianoc · 2022-09-20T02:17:57Z

Also, where is {...x} allowed to appear? I guess only once? Does it need to be at the end of other props?

Only 1 would be fine, I think.

I would double check that adding more than 1 is a parse error. And if not, make it a parse error.

So it could e.g. be encoded as an array of length 1 with a jsx attribute on it.
Both at the parsing and printing level.
Does it make sense?

If I understand correctly, parsing the {...str} as Pexp_array [str] instead of labelled argument, right?

Yes, and add the jsx attribute too.

cristianoc · 2022-09-20T02:21:06Z

See here:

  let jsxExpr =
    match p.Parser.token with
    | Lident _ | Uident _ -> parseJsxOpeningOrSelfClosingElement ~startPos p
    | GreaterThan ->
      (* fragment: <> foo </> *)
      parseJsxFragment p
    | _ -> parseJsxName p
  in
  Parser.eatBreadcrumb p;
  {jsxExpr with pexp_attributes = [jsxAttr]}

One code path calls parseJsxFragment p and the attribute is added at the end.
It could be your code path falls into this kind of situation. Or it could be that you need to add the attribute directly.

mununki · 2022-09-20T02:37:28Z

See here:
  let jsxExpr =
    match p.Parser.token with
    | Lident _ | Uident _ -> parseJsxOpeningOrSelfClosingElement ~startPos p
    | GreaterThan ->
      (* fragment: <> foo </> *)
      parseJsxFragment p
    | _ -> parseJsxName p
  in
  Parser.eatBreadcrumb p;
  {jsxExpr with pexp_attributes = [jsxAttr]}
One code path calls parseJsxFragment p and the attribute is added at the end. It could be your code path falls into this kind of situation. Or it could be that you need to add the attribute directly.

Can I look into it and make PR to improve? Parsing and printing. In my opinion, the spreadProps as labelled argument wouldn't bother users too much. If you think it is not an urgent issue, I'd like to fix this, maybe need some time to finish it.

cristianoc · 2022-09-20T02:39:45Z

Not super urgent.
Did not understand if you're suggesting to fix it now, or to postpone?

mununki · 2022-09-20T02:43:27Z

Not super urgent. Did not understand if you're suggesting to fix it now, or to postpone?

Not postpone, I'll fix it now. But I'm afraid to take some time, not like you super fast problem solver. Actually I feel like I don't understand the code base around the typecore and printer. I'd like to make dynamic import implementation next, so I need to understand more in depth about printer(js emitter).

cristianoc · 2022-09-20T02:44:45Z

Not super urgent. Did not understand if you're suggesting to fix it now, or to postpone?

Not postpone, I'll fix it now. But I'm afraid to take some time, not like you super fast problem solver. Actually I feel like I don't understand the code base around the typecore and printer. I'd like to make dynamic import implementation, so I need to understand more in depth about printer(js emitter).

How about I do it?

mununki · 2022-09-20T02:47:11Z

How about I do it?

Alright you can do it and I can follow what you are going to fix.

cristianoc · 2022-09-20T02:53:42Z

OK turns out my suggestion does not fit, as props need to be (label, expression) pairs. Will do differently.

cristianoc · 2022-09-20T03:06:12Z

Would you take a look rescript-lang/syntax#644

cristianoc · 2022-09-20T03:17:38Z

What's the last item "Spread props without any other props"?

Getting to the point of doing another round of reviews for JSX V4 (after publishing alpha releases).
Any things left to do?

mununki · 2022-09-20T04:29:18Z

Would you take a look rescript-lang/syntax#644

Really nice. I'll learn how to printer works from your PR.
Does it seem to make the labeled argument of spreadProps to _spreadProps?

What's the last item "Spread props without any other props"?

Currently, spreadProps is not available to use solely. The spreadProps is used to update the props record. I can fix it soon.

Any things left to do?

I'll fix "Spread props without any other props" soon and let you know.

mununki · 2022-09-20T09:27:46Z

Any things left to do?

I'll fix "Spread props without any other props" soon and let you know.

One last thing to do rescript-lang/syntax#645

cristianoc · 2022-09-22T10:56:59Z

Looks like it's all done now.

mununki · 2022-09-22T10:57:48Z

Yes! 👍

cristianoc added this to the v10.1 milestone Sep 11, 2022

cristianoc added a commit that referenced this issue Sep 15, 2022

Add missing toplevel "jsx" to build schema.

eddd2e2

See #5646

mununki mentioned this issue Sep 16, 2022

Remove key prop from JSX props type rescript-lang/syntax#635

Merged

cristianoc closed this as completed Sep 23, 2022

Feedback from the forum for JSX v4 #5646

Feedback from the forum for JSX v4 #5646

Comments

mununki commented Sep 7, 2022 • edited by cristianoc Loading

Fixes

Improvements

cristianoc commented Sep 8, 2022

cristianoc commented Sep 8, 2022

mununki commented Sep 10, 2022

cristianoc commented Sep 10, 2022

cristianoc commented Sep 12, 2022

mununki commented Sep 12, 2022 • edited Loading

mununki commented Sep 12, 2022 • edited Loading

cristianoc commented Sep 12, 2022 • edited Loading

cristianoc commented Sep 12, 2022

mununki commented Sep 12, 2022

mununki commented Sep 12, 2022

cristianoc commented Sep 12, 2022 • edited Loading

cristianoc commented Sep 12, 2022

cristianoc commented Sep 12, 2022 • edited Loading

cristianoc commented Sep 12, 2022 • edited Loading

mununki commented Sep 12, 2022 • edited Loading

mununki commented Sep 12, 2022

mununki commented Sep 12, 2022 • edited Loading

cristianoc commented Sep 12, 2022

mununki commented Sep 12, 2022

cristianoc commented Sep 12, 2022

mununki commented Sep 13, 2022

mununki commented Sep 13, 2022

cristianoc commented Sep 13, 2022

cristianoc commented Sep 13, 2022

cristianoc commented Sep 13, 2022

cristianoc commented Sep 13, 2022

cristianoc commented Sep 13, 2022

mununki commented Sep 13, 2022

cristianoc commented Sep 18, 2022

cristianoc commented Sep 19, 2022

mununki commented Sep 19, 2022 • edited Loading

cristianoc commented Sep 20, 2022

mununki commented Sep 20, 2022

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022 • edited Loading

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022

mununki commented Sep 20, 2022

cristianoc commented Sep 20, 2022 • edited Loading

cristianoc commented Sep 20, 2022 • edited Loading

mununki commented Sep 20, 2022

cristianoc commented Sep 20, 2022

mununki commented Sep 20, 2022 • edited Loading

cristianoc commented Sep 20, 2022

mununki commented Sep 20, 2022

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022

cristianoc commented Sep 20, 2022

mununki commented Sep 20, 2022 • edited Loading

mununki commented Sep 20, 2022

cristianoc commented Sep 22, 2022

mununki commented Sep 22, 2022

mununki commented Sep 7, 2022 •

edited by cristianoc

Loading

mununki commented Sep 12, 2022 •

edited

Loading

mununki commented Sep 12, 2022 •

edited

Loading

cristianoc commented Sep 12, 2022 •

edited

Loading

cristianoc commented Sep 12, 2022 •

edited

Loading

cristianoc commented Sep 12, 2022 •

edited

Loading

cristianoc commented Sep 12, 2022 •

edited

Loading

mununki commented Sep 12, 2022 •

edited

Loading

mununki commented Sep 12, 2022 •

edited

Loading

mununki commented Sep 19, 2022 •

edited

Loading

cristianoc commented Sep 20, 2022 •

edited

Loading

cristianoc commented Sep 20, 2022 •

edited

Loading

cristianoc commented Sep 20, 2022 •

edited

Loading

mununki commented Sep 20, 2022 •

edited

Loading

mununki commented Sep 20, 2022 •

edited

Loading