rescript-lang / syntax

ReScript's syntax as a standalone repo.
MIT License
254 stars 38 forks source link

Make empty props as generic type #640

Closed mununki closed 2 years ago

mununki commented 2 years ago

This PR fixes the inconsistency of props type depending on the count of props.

// no props
type props = {}

// props >= 1
type props<'a, 'b, ...> = {a: 'a, b: 'b, ...}

Therefore, it causes the type error

module A = {
  type props = {}
}

let c = React.createElement(A.make, Jsx.addKeyProp({}: A.props<_>, "key")) // Error
// The type A.props is not generic so expects no arguments,
// but is here applied to 1 argument(s)

Simply, dropping the type annotation from the first argument of Jsx.addKeyProp is not solving a problem. If it doesn't have the type annotation A.props<_>, it causes another case of type error.

module A = {
  type props<'x> = {x: 'x}
}

let c = React.createElement(A.make, Jsx.addKeyProp({x: "x"}, "key")) // Error
// record can't find ...

The solution is making the props type as generic even it has no props at all.

type props<_> = {}
let make = (_: props<_>) => { ... }
mununki commented 2 years ago

Haven't noticed the error in case of empty props until I ran the latest master on the larger project, especially after removing the key field from it. 😓

cristianoc commented 2 years ago

Perhaps there's another way.

First, the type of addKeyProp is not correct: it should return the prop type, so it needs a type constraint

let addKeyProp (o : 'props) (k : string) : 'props =
  Obj.magic (Js.Obj.assign (Obj.magic o) [%obj { key = k }])
  [@@inline]

Second, adding a createElementWithKey with the appropriate type seems to take care of the problem: this compiles fine

module C = {
  @react.component
  let make = () => React.null
}

module D = {
  @react.component
  let make = (~x="", ~y="") => React.string(x++y)
}

let createElementWithKey = (comp, props, key) => React.createElement(comp, Jsx.addKeyProp(props, key))

let c1 = React.createElement(C.make, {})
let c2 = createElementWithKey(C.make, {}, "")
let d1 = React.createElement(D.make, {})
let d2 = createElementWithKey(D.make,{}, "")
let d3 = createElementWithKey(D.make, {x:""}, "")

So another way to go is to expose e.g. Jsx. createElementWithKey of the appropriate type instead of Jsx.addKeyProp. The type being (React.component<'a>, 'a, string) => React.element.

mununki commented 2 years ago

Perhaps there's another way.

First, the type of addKeyProp is not correct: it should return the prop type, so it needs a type constraint

let addKeyProp (o : 'props) (k : string) : 'props =
  Obj.magic (Js.Obj.assign (Obj.magic o) [%obj { key = k }])
  [@@inline]

Second, adding a createElementWithKey with the appropriate type seems to take care of the problem: this compiles fine

module C = {
  @react.component
  let make = () => React.null
}

module D = {
  @react.component
  let make = (~x="", ~y="") => React.string(x++y)
}

let createElementWithKey = (comp, props, key) => React.createElement(comp, Jsx.addKeyProp(props, key))

let c1 = React.createElement(C.make, {})
let c2 = createElementWithKey(C.make, {}, "")
let d1 = React.createElement(D.make, {})
let d2 = createElementWithKey(D.make,{}, "")
let d3 = createElementWithKey(D.make, {x:""}, "")

So another way to go is to expose e.g. Jsx. createElementWithKey of the appropriate type instead of Jsx.addKeyProp. The type being (React.component<'a>, 'a, string) => React.element.

Wow! It looks more clean. I’ll fix it this way.

mununki commented 2 years ago

It is working nicely in the example project. https://github.com/rescript-lang/syntax/pull/641