Fix react comp key type #693

mununki · 2022-10-21T17:46:47Z

This PR fixes rescript-lang/rescript-react#73

cristianoc

add a line to the changelog
how about JSXV4.md, does it need updating?
how about adding an example with ?key

cristianoc · 2022-10-21T18:18:21Z

tests/ppx/react/expected/externalWithCustomName.res.txt

@@ -36,4 +36,4 @@ module Foo = {
  external component: React.componentLike<props<int, string>, React.element> = "component"
 }

-let t = React.jsx(Foo.component, {a: 1, b: "1"})
+let t = React.jsxWithKey(Foo.component, {a: 1, b: "1"})


why jsxWithKey if the key is not passed?

https://github.com/rescript-lang/rescript-react/blob/5e09771ae19b52bdb50061da15265b1093b6b61b/src/React.res#L51:L55
I think I should rename this function. 🤔

cristianoc · 2022-10-21T18:18:48Z

tests/ppx/react/expected/forwardRef.res.txt

@@ -167,7 +167,10 @@ module V4A = {
      "div",
      {
        children: ?ReactDOM.someElement(
-          React.jsx(FancyInput.make, {ref: input, children: {React.string("Click to focus")}}),
+          React.jsxWithKey(


why jsxWithKey if the key is not passed?

cristianoc · 2022-10-21T18:19:39Z

tests/ppx/react/expected/spreadProps.res.txt

@@ -19,7 +19,7 @@ let c2 = React.createElement(A.make, {...p, x: "x"})
 // let c0 = <A x="x" {...p0} {...p1} />

 // only spread props
-let c1 = React.jsx(A.make, p)
+let c1 = React.jsxWithKey(A.make, p)


why jsxWithKey is the key is not passed?

cristianoc · 2022-10-21T18:19:48Z

tests/ppx/react/expected/spreadProps.res.txt


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


why jsxWithKey is the key is not passed?

mununki · 2022-10-22T03:39:37Z

@cristianoc I think I'm done with this PR including:

making key type an optional arg of React binding for both classic and automatic
updating v4 spec with example

cristianoc

Thanks.

Would you add an example with <Foo ?key/>
Update the changelog

tests/ppx/react/expected/spreadProps.res.txt

cknitt · 2022-10-22T07:45:45Z

cli/JSXV4.md

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


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

Good catch! I'll fix it.

cknitt · 2022-10-22T07:51:35Z

Also, I was just wondering about the following:

In

addKeyProp(~key="42", {a: a, b: b /* and maybe many more props */})

we are creating the props object, it is not passed from outside. So why do we need to use Obj.assign to create a copy of it instead of just mutating it?

mununki · 2022-10-22T09:00:51Z

Would you add an example with <Foo ?key/>

Do you mean examples in the spec? or test?

cristianoc · 2022-10-22T11:09:06Z

Would you add an example with <Foo ?key/>

Do you mean examples in the spec? or test?

test

mununki · 2022-10-22T15:44:57Z

Also, I was just wondering about the following:

In
addKeyProp(~key="42", {a: a, b: b /* and maybe many more props */})
we are creating the props object, it is not passed from outside. So why do we need to use Obj.assign to create a copy of it instead of just mutating it?

Not sure I understand what you mean.
I think using Obj.assign inline would be a little verbose and more tricky to annotate type to put some magic on it. The props type doesn't have a key field, but it has to check the key type and manipulate it.

cristianoc

Looks good to me.
@mattdamon108 does everything work with your project?

cristianoc · 2022-10-22T17:39:26Z

Also, I was just wondering about the following:

In
addKeyProp(~key="42", {a: a, b: b /* and maybe many more props */})
we are creating the props object, it is not passed from outside. So why do we need to use Obj.assign to create a copy of it instead of just mutating it?

The mechanism used does 2 things:

type checks the normal props and generates the record value for them
gets around the type checker for key, so it does not have to be part of the user-visible prop type definition, and extends the record in the generated code

Even though possible in principle, I'm not sure that both are possible at the same time currently, short of creating some new primitive with special runtime support.

cristianoc · 2022-10-22T17:41:09Z

Oh maybe the question is only about imperatively adding the key property. That should be OK.

cknitt · 2022-10-22T19:32:40Z

Yes, I meant that Obj.assign is unnecessarily copying the props object, and that we could just do

(props->Obj.magic)["key"] = key

instead.

mununki · 2022-10-23T05:30:00Z

Looks good to me. @mattdamon108 does everything work with your project?

Now I tested v3 and v4 classic & automatic, it works fine!

mununki · 2022-10-23T05:37:18Z

Yes, I meant that Obj.assign is unnecessarily copying the props object, and that we could just do
(props->Obj.magic)["key"] = key
instead.

Maybe this? rescript-lang/rescript-react@2f48356

mununki · 2022-10-23T05:45:26Z

Yes, I meant that Obj.assign is unnecessarily copying the props object, and that we could just do
(props->Obj.magic)["key"] = key
instead.
Maybe this? rescript-lang/rescript-react@2f48356

It also works fine on tests.

mununki · 2022-10-23T05:48:07Z

One impressive thing is that I don't need to change the existing codes between v3 and v4 classic and automatic, except for the Context provider component in my company code base which is a quite large project.
https://github.com/rescript-lang/syntax/blob/master/cli/JSXV4.md#reactcontext

cknitt · 2022-10-23T06:58:32Z

Yes, I meant that Obj.assign is unnecessarily copying the props object, and that we could just do
(props->Obj.magic)["key"] = key
instead.
Maybe this? rescript-lang/rescript-react@2f48356
It also works fine on tests.

We need to ensure that we do not mutate objects passed from user code. So if we do the mutation in createElementWithKey and createElementVariadicWithKey then we should document that these functions are for use by the PPX only and not meant to be called from user code.

Also, what happens in the case

<A {...props} key />

? Is props copied before being mutated or just passed through?

cknitt · 2022-10-23T07:01:15Z

Are there no tests for createElementVariadicWithKey?

cristianoc · 2022-10-23T07:22:06Z

There's this too https://reactjs.org/docs/react-api.html#cloneelement

mununki · 2022-10-23T07:29:46Z

Yes, I meant that Obj.assign is unnecessarily copying the props object, and that we could just do
(props->Obj.magic)["key"] = key
instead.
Maybe this? rescript-lang/rescript-react@2f48356
It also works fine on tests.
We need to ensure that we do not mutate objects passed from user code. So if we do the mutation in createElementWithKey and createElementVariadicWithKey then we should document that these functions are for use by the PPX only and not meant to be called from user code.

Also, what happens in the case
<A {...props} key />
? Is props copied before being mutated or just passed through?

This is a very good point. <A {...props} key /> would manipulate the usrs' props object.

mununki · 2022-10-23T07:30:56Z

@cknitt Can you move this topic to rescript-react? rescript-lang/rescript-react#74

mununki added 2 commits October 22, 2022 02:45

fix react comp key type

bff8745

add tests

3a12ce1

cristianoc reviewed Oct 21, 2022

View reviewed changes

mununki added 6 commits October 22, 2022 11:49

call renamed jsx binding

6881b10

add test

16203d7

add more examples

30bab47

fix key type of lowercase jsx runtime

556bda4

add more examples

e4b056e

update jsx v4 spec about key type

1b92972

mununki mentioned this pull request Oct 22, 2022

Version 0.11.0-rc.2 rescript-lang/rescript-react#72

Merged

cristianoc reviewed Oct 22, 2022

View reviewed changes

tests/ppx/react/expected/spreadProps.res.txt Show resolved Hide resolved

cknitt reviewed Oct 22, 2022

View reviewed changes

mununki added 3 commits October 23, 2022 00:56

fix JSX v4 spec

c677540

update changelog

03c1467

add jsx key type tests

7bf775c

cristianoc approved these changes Oct 22, 2022

View reviewed changes

cristianoc added this to the v10.1 "The New Features" milestone Oct 22, 2022

cristianoc merged commit eb059d4 into master Oct 23, 2022

cristianoc deleted the fix-key-type branch October 23, 2022 06:08

mununki mentioned this pull request Oct 23, 2022

Fix key prop type rescript-lang/rescript-react#74

Merged

cristianoc mentioned this pull request Oct 30, 2022

Create v10.1 docs rescript-association/rescript-lang.org#568

Closed

57 tasks

mununki mentioned this pull request Aug 29, 2023

Explore to rewrite Jsx in res, add helper functions in the compiler rescript-lang/rescript-compiler#5757

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix react comp key type #693

Fix react comp key type #693

mununki commented Oct 21, 2022

cristianoc left a comment

cristianoc Oct 21, 2022

mununki Oct 21, 2022

cristianoc Oct 21, 2022

cristianoc Oct 21, 2022

cristianoc Oct 21, 2022

mununki commented Oct 22, 2022

cristianoc left a comment

cknitt Oct 22, 2022

mununki Oct 22, 2022

cknitt commented Oct 22, 2022

mununki commented Oct 22, 2022

cristianoc commented Oct 22, 2022

mununki commented Oct 22, 2022

cristianoc left a comment

cristianoc commented Oct 22, 2022

cristianoc commented Oct 22, 2022

cknitt commented Oct 22, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

cknitt commented Oct 23, 2022

cknitt commented Oct 23, 2022

cristianoc commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

Fix react comp key type #693

Fix react comp key type #693

Conversation

mununki commented Oct 21, 2022

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc Oct 21, 2022

Choose a reason for hiding this comment

mununki Oct 21, 2022

Choose a reason for hiding this comment

cristianoc Oct 21, 2022

Choose a reason for hiding this comment

cristianoc Oct 21, 2022

Choose a reason for hiding this comment

cristianoc Oct 21, 2022

Choose a reason for hiding this comment

mununki commented Oct 22, 2022

cristianoc left a comment

Choose a reason for hiding this comment

cknitt Oct 22, 2022

Choose a reason for hiding this comment

mununki Oct 22, 2022

Choose a reason for hiding this comment

cknitt commented Oct 22, 2022

mununki commented Oct 22, 2022

cristianoc commented Oct 22, 2022

mununki commented Oct 22, 2022

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc commented Oct 22, 2022

cristianoc commented Oct 22, 2022

cknitt commented Oct 22, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022

cknitt commented Oct 23, 2022

cknitt commented Oct 23, 2022

cristianoc commented Oct 23, 2022

mununki commented Oct 23, 2022

mununki commented Oct 23, 2022