Feedback from the forum for JSX v4

[mununki]

Feedback from the forum for JSX v4 https://forum.rescript-lang.org/t/call-for-help-test-the-react-jsx-v4/3713

Fixes

• [x] Formatting {...str} to spreadProps=str
• [ ] Spread props without any other props https://github.com/rescript-lang/syntax/pull/645
• [x] Not passing key to the component in V4 + classic https://github.com/rescript-lang/syntax/pull/635 https://github.com/rescript-lang/syntax/pull/641
• [x] Handle the ref in props type: external binding https://forum.rescript-lang.org/t/call-for-help-test-the-react-jsx-v4/3713/57?u=moondaddi https://github.com/rescript-lang/syntax/pull/637

Improvements

• [ ] Update the migration doc
  • [ ] Breaking changes between v3 and v4
  • [ ] Explanation about jsx configuration, e.g. what the classic and automatic means
• [ ] Update the spec docs
  • [ ] Spread props
• [x] Error message about missing props: better phrasing https://github.com/rescript-lang/rescript-compiler/pull/5674
• [x] Error message about missing props: mention all the props missing https://github.com/rescript-lang/rescript-compiler/pull/5657
• [x] Type information about props type in the editor. This is not specific about JSX but follows from https://github.com/rescript-lang/rescript-vscode/issues/557
• [x] Update json schema for bsconfig.json https://github.com/rescript-lang/rescript-compiler/pull/5661/commits/0d0333d56d008c24b6fe7e0a3051d407aadda964
• [x] Error message about type error in props: special case use wrapper memo #5635

[cristianoc]

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

[cristianoc]

@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]

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]

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]

@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]

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]

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]

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]

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

https://github.com/rescript-lang/rescript-compiler/pull/5658

[mununki]

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]

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

[cristianoc]

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

~deleted confused comment~

[cristianoc]

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]

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]

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]

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]

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

[mununki]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

Actually the empty record type type empty = {} was already there in the syntax, but disabled. Now added to the type checker too: https://github.com/rescript-lang/rescript-compiler/pull/5658

[mununki]

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.

[cristianoc]

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.