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

Fix react comp key type #693

Merged
merged 11 commits into from
Oct 23, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions cli/JSXV4.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ To build an entire project in V4 mode, including all its dependencies, use the n
"jsx": { "version": 4 }
```

> Note that JSX V4 requires the rescript compiler 10.1 or higher, and `rescript-react` version `0.11` or higher. In addition, `react` version `18.2` is required.
> Note that JSX V4 requires the rescript compiler 10.1 or higher, and `rescript-react` version `0.11` or higher. In addition, `react` version `18.0` is required.

## Configuration And Upgrade

Expand Down Expand Up @@ -286,7 +286,11 @@ React.createElement(Comp.make, {x, y: 7, ?z})

<Comp x key="7">
// is transformed to
React.createElement(Comp.make, Jsx.addKeyProp({x: x}, "7"))
React.createElement(Comp.make, Jsx.addKeyProp(~key="7", {x: x}))

<Comp x key=?Some("7")>
// is transformed to
React.createElement(Comp.make, Jsx.addKeyProp(~key=?Some("7"), {x: x}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it React.addKeyProp as per rescript-lang/rescript-react#74?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I'll fix it.

```

### New experimental automatic mode
Expand Down
45 changes: 18 additions & 27 deletions cli/reactjs_jsx_v4.ml
Original file line number Diff line number Diff line change
Expand Up @@ -387,49 +387,42 @@ let transformUppercaseCall3 ~config modulePath mapper jsxExprLoc callExprLoc
| "automatic" ->
let jsxExpr, key =
match (!childrenArg, keyProp) with
| None, (_, keyExpr) :: _ ->
( Exp.ident
{loc = Location.none; txt = Ldot (Lident "React", "jsxKeyed")},
[(nolabel, keyExpr)] )
| None, key :: _ ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsx")},
[key] )
| None, [] ->
(Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsx")}, [])
| Some _, (_, keyExpr) :: _ ->
( Exp.ident
{loc = Location.none; txt = Ldot (Lident "React", "jsxsKeyed")},
[(nolabel, keyExpr)] )
| Some _, key :: _ ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsxs")},
[key] )
| Some _, [] ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "React", "jsxs")},
[] )
in
Exp.apply ~attrs jsxExpr ([(nolabel, makeID); (nolabel, props)] @ key)
Exp.apply ~attrs jsxExpr (key @ [(nolabel, makeID); (nolabel, props)])
| _ -> (
match (!childrenArg, keyProp) with
| None, (_, keyExpr) :: _ ->
| None, key :: _ ->
Exp.apply ~attrs
(Exp.ident
{
loc = Location.none;
txt = Ldot (Lident "React", "createElementWithKey");
})
[(nolabel, makeID); (nolabel, props); (nolabel, keyExpr)]
[key; (nolabel, makeID); (nolabel, props)]
| None, [] ->
Exp.apply ~attrs
(Exp.ident
{loc = Location.none; txt = Ldot (Lident "React", "createElement")})
[(nolabel, makeID); (nolabel, props)]
| Some children, (_, keyExpr) :: _ ->
| Some children, key :: _ ->
Exp.apply ~attrs
(Exp.ident
{
loc = Location.none;
txt = Ldot (Lident "React", "createElementVariadicWithKey");
})
[
(nolabel, makeID);
(nolabel, props);
(nolabel, children);
(nolabel, keyExpr);
]
[key; (nolabel, makeID); (nolabel, props); (nolabel, children)]
| Some children, [] ->
Exp.apply ~attrs
(Exp.ident
Expand Down Expand Up @@ -497,23 +490,21 @@ let transformLowercaseCall3 ~config mapper jsxExprLoc callExprLoc attrs
in
let jsxExpr, key =
match (!childrenArg, keyProp) with
| None, (_, keyExpr) :: _ ->
( Exp.ident
{loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxKeyed")},
[(nolabel, keyExpr)] )
| None, key :: _ ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsx")},
[key] )
| None, [] ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsx")},
[] )
| Some _, (_, keyExpr) :: _ ->
( Exp.ident
{loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxsKeyed")},
[(nolabel, keyExpr)] )
| Some _, key :: _ ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxs")},
[key] )
| Some _, [] ->
( Exp.ident {loc = Location.none; txt = Ldot (Lident "ReactDOM", "jsxs")},
[] )
in
Exp.apply ~attrs jsxExpr
([(nolabel, componentNameExpr); (nolabel, props)] @ key)
(key @ [(nolabel, componentNameExpr); (nolabel, props)])
| _ ->
let children, nonChildrenProps =
extractChildren ~loc:jsxExprLoc callArguments
Expand Down
40 changes: 38 additions & 2 deletions tests/ppx/react/expected/noPropsWithKey.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ module V4C = {
ReactDOM.createElement(
React.fragment,
[
React.createElementWithKey(V4CA.make, {}, "k"),
React.createElementWithKey(V4CB.make, {}, "k"),
React.createElementWithKey(~key="k", V4CA.make, {}),
React.createElementWithKey(~key="k", V4CB.make, {}),
],
)
let make = {
Expand All @@ -36,3 +36,39 @@ module V4C = {
\"NoPropsWithKey$V4C"
}
}

@@jsxConfig({version: 4, mode: "automatic"})

module V4CA = {
type props = {}

@react.component let make = (_: props) => ReactDOM.jsx("div", {})
let make = {
let \"NoPropsWithKey$V4CA" = props => make(props)

\"NoPropsWithKey$V4CA"
}
}

module V4CB = {
type props = {}

@module("c")
external make: React.componentLike<props, React.element> = "component"
}

module V4C = {
type props = {}

@react.component
let make = (_: props) =>
React.jsxs(
React.jsxFragment,
{children: [React.jsx(~key="k", V4CA.make, {}), React.jsx(~key="k", V4CB.make, {})]},
)
let make = {
let \"NoPropsWithKey$V4C" = props => make(props)

\"NoPropsWithKey$V4C"
}
}
4 changes: 2 additions & 2 deletions tests/ppx/react/expected/removedKeyProp.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ let make = (_: props) =>
ReactDOM.createElement(
React.fragment,
[
React.createElementWithKey(Foo.make, {x: "x", y: "y"}, "k"),
React.createElementWithKey(~key="k", Foo.make, {x: "x", y: "y"}),
React.createElement(Foo.make, {x: "x", y: "y"}),
React.createElementWithKey(
~key="k",
HasChildren.make,
{
children: React.createElement(Foo.make, {x: "x", y: "y"}),
},
"k",
),
React.createElement(
HasChildren.make,
Expand Down
20 changes: 20 additions & 0 deletions tests/ppx/react/expected/spreadProps.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@ let c1 = React.createElement(A.make, p)
// reversed order
let c2 = React.createElement(A.make, {...p, x: "x"})

let c3 = ReactDOM.createDOMElementVariadic("div", ~props=p, [])

let c4 = ReactDOM.createDOMElementVariadic("div", ~props={...p, x: "x", key: "k"}, [])

let c4 = ReactDOM.createDOMElementVariadic(
"div",
~props={...p, key: "k"},
[ReactDOM.createDOMElementVariadic("br", []), ReactDOM.createDOMElementVariadic("br", [])],
)

@@jsxConfig({version: 4, mode: "automatic"})
// Error: spreadProps should be first in order than other props
// let c0 = <A x="x" {...p} />
Expand All @@ -23,3 +33,13 @@ let c1 = React.jsx(A.make, p)

// reversed order
let c2 = React.jsx(A.make, {...p, x: "x"})

let c3 = ReactDOM.jsx("div", p)

let c4 = ReactDOM.jsx(~key="k", "div", {...p, x: "x"})

let c5 = ReactDOM.jsxs(
~key="k",
"div",
{...p, children: React.array([ReactDOM.jsx("br", {}), ReactDOM.jsx("br", {})])},
)
17 changes: 17 additions & 0 deletions tests/ppx/react/noPropsWithKey.res
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,23 @@ module V4CB = {
external make: unit => React.element = "component"
}

module V4C = {
@react.component
let make = () => <><V4CA key="k" /> <V4CB key="k" /></>
}

@@jsxConfig({version: 4, mode: "automatic"})

module V4CA = {
@react.component
let make = () => <div />
}

module V4CB = {
@module("c") @react.component
external make: unit => React.element = "component"
}

module V4C = {
@react.component
let make = () => <><V4CA key="k" /> <V4CB key="k" /></>
Expand Down
12 changes: 12 additions & 0 deletions tests/ppx/react/spreadProps.res
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ let c1 = <A {...p} />
// reversed order
let c2 = <A {...p} x="x" />

let c3 = <div {...p} />

let c4 = <div {...p} x="x" key="k" />

let c4 = <div {...p} key="k"><br /><br /></div>

@@jsxConfig({version:4, mode: "automatic"})
// Error: spreadProps should be first in order than other props
// let c0 = <A x="x" {...p} />
Expand All @@ -23,3 +29,9 @@ let c1 = <A {...p} />

// reversed order
let c2 = <A {...p} x="x" />

let c3 = <div {...p} />

let c4 = <div {...p} x="x" key="k" />

let c5 = <div {...p} key="k"><br /><br /></div>