React fragment's children type change and JSX4 adaptation #6238

mununki · 2023-05-02T18:18:20Z

This PR modifies the JSX4 implementation to accommodate the change of React fragment's 'children type to React.element rescript-lang/rescript-react#94. The main changes are as follows:

JSX4 classic

Using React* instead of ReactDOM*

Adding an empty record as a prop for React.createElementVariadic

let _ = React.createElementVariadic(
	React.fragment,
	{}, // along with type change of React.fragment
	[ReactDOM.createDOMElementVariadic("div", []), ReactDOM.createDOMElementVariadic("div", [])],
)

~~Adding React.array for children of fragment~~

JSX4 automatic:
1. Adding a missing React.array for children of fragment

~~Side change: make test-syntax wouldn't build the compiler, so I've changed it to build compiler first and test-syntax.~~

mununki · 2023-05-02T18:22:08Z

@cknitt Please refer to the test output, fragment.res.txt, showing the changes in the transformation result. Can you test it on your project? We need to ensure this change doesn't bring the runtime error.

mununki · 2023-05-02T18:28:04Z

In my test project, the build seems to work well, and it also appears to work correctly at runtime.

cristianoc · 2023-05-02T18:35:37Z

Are any of these changes something genType should be aware of?

mununki · 2023-05-02T18:40:09Z

Are any of these changes something genType should be aware of?

Oops, I couldn't think about the genType. Let me check. This is going to be a good chance for me to learn the genType implementation.

cknitt · 2023-05-02T18:53:07Z

Makefile

@@ -26,7 +26,7 @@ node_modules/.bin/semver:
 test: lib
 	node scripts/ciTest.js -all

-test-syntax:
+test-syntax: build


This will cause CI to run the dune build + copyExe.js twice.

I should probably clean this up somehow at some point, but for this PR I'd prefer to keep the Makefile as it is.

Ah, noted. Reverted 9af6b6a

cknitt · 2023-05-02T19:01:00Z

My current (large) project now builds again with this change, and I see no changes in the compiler output! 🎉 Thanks a lot!

My project is in uncurried mode though, it won't build without, so I haven't tested that yet.
Will try that in another project tomorrow.

cknitt · 2023-05-03T05:52:16Z

@mununki Unfortunately our ReScript 11 project only builds with uncurried and JSX 4, and our other projects are not ReScript 11 ready yet.

So it's hard for me to test JSX 3 and curried mode. Do you have a project/setup where you could try this?

mununki · 2023-05-03T06:46:40Z

Wow, it seems you've already migrated your project to uncurried already! nice!
I have an archived project to test JSX3, I'll test it. Can you test it on your project with classic?

cknitt · 2023-05-03T07:09:01Z

With JSX4 classic, when using <>, I get

  The record field children can't be found.

  If it's defined in another module or file, bring it into scope by:
  - Prefixing it with said module name: TheModule.children
  - Or specifying its type:
  let theValue: TheModule.theType = {children: VALUE}

cknitt · 2023-05-03T07:10:45Z

But only without the changes I made in @rescript/react, with those changes it works.

cknitt · 2023-05-03T07:12:02Z

Ah that's clear because there were no children there previously:

@module("react") external fragment: 'a = "Fragment"

So everything looks good, provided @rescript/react is upgraded, too.

cknitt · 2023-05-03T07:15:37Z

Oh no, in classic mode it seems this gives key warnings at runtime for the children array though. 😞

mununki · 2023-05-03T08:05:48Z

With JSX4 classic, when using <>, I get

  The record field children can't be found.

  If it's defined in another module or file, bring it into scope by:
  - Prefixing it with said module name: TheModule.children
  - Or specifying its type:
  let theValue: TheModule.theType = {children: VALUE}

Yes, it is intended. Now the ppx is looking for the children for fragment, that's why.

mununki · 2023-05-03T08:06:50Z

Oh no, in classic mode it seems this gives key warnings at runtime for the children array though. 😞

Can you share the example code?

mununki · 2023-05-03T08:49:17Z

<>
  <div />
  <div />
</>

If we have key warning from this case, it is not related to fragment. <div /> needs to have key.

cknitt · 2023-05-03T09:32:30Z

Yes, it occurs on a simple example like this:

<>
  <div />
  <div />
</>

The divs should not need a key in this case and did not need one before, but with your changes they now do because React.createElement is created differently.

I found this stackoverflow entry that also describes this: https://stackoverflow.com/questions/64889193/react-key-on-array-of-children-versus-multiple-hard-coded-children

mununki · 2023-05-03T09:42:24Z

Yes, it occurs on a simple example like this:
<>
  <div />
  <div />
</>
The divs should not need a key in this case and did not need one before, but with your changes they now do because React.createElement is created differently.

I found this stackoverflow entry that also describes this: stackoverflow.com/questions/64889193/react-key-on-array-of-children-versus-multiple-hard-coded-children

Ah, you're correct. It should not require the key for <div /> unless we are iterating Array.map(_ => <div />). This is why we need React.createElementVariadic. I'm going to fix the ppx for this case.

mununki · 2023-05-03T11:57:03Z

@cknitt Fixed missing key warning for JSX4 classic fragment, could you test it again?

mununki · 2023-05-03T12:16:56Z

Are any of these changes something genType should be aware of?

@cristianoc The type of React.fragment has changed, but since the type of React.fragment's application remains the same as React.element, I couldn't find anything that would affect genType.

cristianoc · 2023-05-03T12:34:32Z

Are any of these changes something genType should be aware of?

@cristianoc The type of React.fragment has changed, but since the type of React.fragment's application remains the same as React.element, I couldn't find anything that would affect genType.

Perfect. Thank you for checking.

cknitt · 2023-05-03T13:31:17Z

@mununki Classic mode now works without any runtime warnings 🎉.

However, there is still the following change in the generated code for <>:

Previously:

return React.createElement(React.Fragment, undefined, ... /* the children */);

Now

return React.createElement(React.Fragment, {}, ... /* the children */);

(Not sure if that is an issue.)

mununki · 2023-05-03T15:32:13Z

@mununki Classic mode now works without any runtime warnings 🎉.

However, there is still the following change in the generated code for <>:

Previously:
return React.createElement(React.Fragment, undefined, ... /* the children */);
Now
return React.createElement(React.Fragment, {}, ... /* the children */);
(Not sure if that is an issue.)

The second one is correct as referring to the doc https://react.dev/reference/react/createElement#:~:text=props%3A%20The%20props,element.key.

The props argument must either be an object or null. If you pass null, it will be treated the same as an empty object.

I think previous output is not correct. 🤷

mununki · 2023-05-03T16:14:08Z

Now that I have the opportunity to look again, it seems that there are some things that need to be fixed in the conversion results for JSX4 classic. For example:
1.

let _ = <><div /></>
// generated
let _ = React.createElement(
  React.fragment,
  {children: ReactDOM.createDOMElementVariadic("div", [])},
)
// should be
let _ = React.createElement(
  React.fragment,
  {}
  ReactDOM.createDOMElementVariadic("div", []),
)

let _ = <Z><div /></Z>
// generated
let _ = React.createElement(Z.make, {children: ReactDOM.createDOMElementVariadic("div", [])})
// should be
let _ = React.createElement(Z.make, {}, ReactDOM.createDOMElementVariadic("div", []))

let _ = <div />
// generated
let _ = ReactDOM.createDOMElementVariadic("div", [])
// should be
let _ = ReactDOM.createDOMElementVariadic("div", {})

mununki · 2023-05-03T17:59:00Z

@cknitt Although there have been no issues at runtime so far, the expression is strictly different from the signature of React.createElement, so I've tried making some modifications regarding the parts I've summarized in my comment above. What do you think?
Of course, build and runtime testing are necessary. I'm also planning to run tests on my fairly large project tomorrow.

EDIT: this is only change for JSX4 classic. The automatic is just fine.

mununki · 2023-05-04T01:13:25Z

The issue lies in the second case. To summarize the issue a bit,

// (JSX4 classic)
// module Z = {
//   @react.component
//   let make = (~children:React.element) => children // children prop is not optional
// }

// application
let _ = <Z> <div /> </Z>

// generated
let _ = React.createElementVariadic(Z.make, {children: React.null}, [<div />])
                                             ^^^^^^^^^^^^^^^^^^^^^

// should be
let _ = React.createElementVariadic(Z.make, {}, [<div />])

If the {children: React.element} corresponding to the prop record is not passed in the second argument position, a build error occurs.

cristianoc

Added some high-level comments to what otherwise seems to be a pretty complete PR.

cristianoc · 2023-05-04T02:09:09Z

jscomp/syntax/src/reactjs_jsx_v4.ml

+    | Exact children -> (
+      match config.React_jsx_common.mode with
+      | "automatic" -> [(labelled "children", children)]
+      | _ ->


Perhaps it's obvious by the discussion, but this is the aspect I'd like to dig into a little bit.
Can you explain again why there are 2 apparently different treatments for automatic vs non-automatic?

Is is possible to explain the need for a difference at a conceptual level, or is this just due to some implementation details? I wonder especially if this reflects a difference in the way React already treats the different modes, or if it is specific to ReScript.

Also why is that a string? Not clear we catch all the cases here, especially if more cases will be added later in addition to the current 3 cases.

I wonder especially if this reflects a difference in the way React already treats the different modes

That's right, React handles the representation of classic and automatic differently with different runtimes. This is easy to see in the Babel playground.

(I couldn't find the share function in the Babel playground, so here's a screenshot.)

Also why is that a string? Not clear we catch all the cases here, especially if more cases will be added later in addition to the current 3 cases.

It doesn't have to be a string, because we take a string as an argument to -bs-jsx-mode, but it could be parsed as a variant. As for the exception handling in the parsing, I think we could use the default value and not have to use the option type.

~~Suddenly, I'm wondering if ReScript should only support the automatic React runtime, since the classic runtime is for React v16 and earlier.~~ I withdraw my idea. We need to preserve backward compatibility.

cristianoc

Since the apparent extra complexity is due to React's choice (separate question is whether that is a good choice, but not relevant for this), everything looks good here, as far as I can see.
Go ahead if there are no pending issues.

cknitt · 2023-05-04T05:36:01Z

@mununki I'll retest the last commit against my project with JSX4 classic.

Could you add to the changelog as a breaking change that @rescript/react >= 0.12.0-alpha.2 is required now?

cknitt · 2023-05-04T05:45:44Z

Tested again and looks good to me.
I'd say let's merge after the CHANGELOG update, and then I can release alpha.6.

mununki · 2023-05-04T06:42:23Z

Tested again and looks good to me. I'd say let's merge after the CHANGELOG update, and then I can release alpha.6.

Thank you so much for the test. I'll test JSX3 and JSX4 classic a bit more in my projects

mununki · 2023-05-04T11:33:15Z

Sorry, my team has been using jsConverter a lot and I'm testing it while fixing it, so it's taking a while. I'll just do some more runtime checks.

mununki · 2023-05-04T18:37:47Z

I found an issue when testing JSX4 classic: my latest implementation of passing children as the 3rd argument has one problem. I can't pass children of type e => React.element as the 3rd argument because it's not the same type as {children: React.null}, which I pass as the 2nd argument. I looked it up and found that, contrary to the signature of React.createElement, it's possible to pass children as the 2nd argument with other props. (In other words, the existing transformation implementation is not wrong.) See https://frontarm.com/james-k-nelson/4-ways-pass-children-react-elements/

I'll revert a couple of recent commits that tried to send children as a 3rd argument and clean up the PR. Sorry for the confusion.

mununki · 2023-05-04T18:41:00Z

Side comment: I was once again surprised by the complexity of the React interface and implementation. I found the automatic runtime to be an improvement on that.

mununki · 2023-05-05T09:23:46Z

I've reverted, cleaned up, and rebased my latest attempt.

mununki · 2023-05-05T09:56:53Z

No build or runtime issue found in JSX4 classic and automatic on my projects

mununki · 2023-05-05T10:00:15Z

Perhaps no need to run make artifacts and commit the artifacts.txt file?

This reverts commit 649a0f7.

mununki requested a review from cknitt May 2, 2023 18:18

cknitt reviewed May 2, 2023

View reviewed changes

cristianoc reviewed May 4, 2023

View reviewed changes

cristianoc approved these changes May 4, 2023

View reviewed changes

cknitt approved these changes May 4, 2023

View reviewed changes

mununki force-pushed the fix-fragment-def-jsx4 branch from 28475ca to 5e9536a Compare May 5, 2023 02:20

mununki added 2 commits May 5, 2023 17:20

add tests

5afd83e

fix JSX to adopt to change of React.fragment type

12d4c08

mununki force-pushed the fix-fragment-def-jsx4 branch from 5e9536a to 12d4c08 Compare May 5, 2023 08:33

artifacts

649a0f7

mununki added 2 commits May 5, 2023 19:00

Revert "artifacts"

e3a67b4

This reverts commit 649a0f7.

stale comment

723e144

mununki merged commit f4d5298 into master May 5, 2023

mununki deleted the fix-fragment-def-jsx4 branch May 5, 2023 13:18

cknitt mentioned this pull request Jun 18, 2023

External binding to component without @react.component #5725

Open

fhammerschmidt mentioned this pull request Oct 2, 2023

Document v11 features. rescript-lang/rescript-lang.org#697

Closed

4 tasks

tristanwagner mentioned this pull request Feb 5, 2024

update @rescript/react to 0.12.0 jihchi/vitejs-template-react-rescript#24

Merged

React fragment's children type change and JSX4 adaptation #6238

React fragment's children type change and JSX4 adaptation #6238

Conversation

mununki commented May 2, 2023 • edited Loading

mununki commented May 2, 2023

mununki commented May 2, 2023

cristianoc commented May 2, 2023

mununki commented May 2, 2023

cknitt May 2, 2023

Choose a reason for hiding this comment

mununki May 3, 2023

Choose a reason for hiding this comment

cknitt commented May 2, 2023

cknitt commented May 3, 2023

mununki commented May 3, 2023

cknitt commented May 3, 2023

cknitt commented May 3, 2023

cknitt commented May 3, 2023

cknitt commented May 3, 2023 • edited Loading

mununki commented May 3, 2023

mununki commented May 3, 2023

mununki commented May 3, 2023

cknitt commented May 3, 2023

mununki commented May 3, 2023

mununki commented May 3, 2023

mununki commented May 3, 2023

cristianoc commented May 3, 2023

cknitt commented May 3, 2023 • edited Loading

mununki commented May 3, 2023

mununki commented May 3, 2023

mununki commented May 3, 2023 • edited Loading

mununki commented May 4, 2023 • edited Loading

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc May 4, 2023

Choose a reason for hiding this comment

mununki May 4, 2023 • edited Loading

Choose a reason for hiding this comment

mununki May 4, 2023 • edited Loading

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

cknitt commented May 4, 2023

cknitt commented May 4, 2023

mununki commented May 4, 2023

mununki commented May 4, 2023

mununki commented May 4, 2023

mununki commented May 4, 2023

mununki commented May 5, 2023

mununki commented May 5, 2023

mununki commented May 5, 2023

mununki commented May 2, 2023 •

edited

Loading

cknitt commented May 3, 2023 •

edited

Loading

cknitt commented May 3, 2023 •

edited

Loading

mununki commented May 3, 2023 •

edited

Loading

mununki commented May 4, 2023 •

edited

Loading

mununki May 4, 2023 •

edited

Loading

mununki May 4, 2023 •

edited

Loading