Skip to content
This repository was archived by the owner on Jun 15, 2023. It is now read-only.

Make empty props as generic type #640

Closed
wants to merge 1 commit into from
Closed

Conversation

mununki
Copy link
Member

@mununki mununki commented Sep 18, 2022

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
Copy link
Member Author

mununki commented Sep 18, 2022

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
Copy link
Contributor

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
Copy link
Member Author

mununki commented Sep 18, 2022

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
Copy link
Member Author

mununki commented Sep 19, 2022

It is working nicely in the example project.
#641

@mununki mununki closed this Sep 19, 2022
@mununki mununki deleted the fix-props-generic branch September 19, 2022 00:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants