Explore to rewrite Jsx in res, add helper functions in the compiler

[mununki]

This PR is to explore how it works about rewriting Jsx module in res and adding helper functions in res module.
I'll relocate helper functions to the separate res module.

[mununki]

Related discussion rescript-lang/rescript-react#76

[cknitt]

Sorry, I don't think the functions belong into the Jsx module, as that module is meant to be framework-independent, but the helper functions are specific to the React PPX.

I think they should go into a new module named ReactPPX4Support or something like that.

[mununki]

Sorry, I don't think the functions belong into the Jsx module, as that module is meant to be framework-independent, but the helper functions are specific to the React PPX.

I think they should go into a new module named ReactPPX4Support or something like that.

Totally agree with you. Let me relocate helper functions to ReactPPX4Support module.

[mununki]

I've tested the newly built compiler on the example project. It works fine.
I'll let you know the test result of the large project.

[mununki]

I've tested in a fairly large project which is my company. It works fine.

Pros:

• rescript-react zero-cost binding

// as-is
import * as React from "@rescript/react/src/React.bs.js";
import * as React$1 from "react";

// to-be: `@rescript/react/...` is not imported anymore
import * as React from "react";
import * as ReactPPX4Support from "rescript/lib/es6/reactPPX4Support.js";

Cons:

• Little unreadable js output in passing React.createElement fun

// as-is
React.createElementWithKey("k1", RemovedKeyProp$Foo, {
                  x: "x",
                  y: "y"
                })

// to-be
ReactPPX4Support.createElementWithKey("k1", (function (prim0, prim1) { // <- here
                    return React.createElement(prim0, prim1);
                  }), RemovedKeyProp$Foo, {
                  x: "x",
                  y: "y"
                })

As we had discussed before, it comes to my head to add rescript-react into the compiler. The unreadable output in passing fun would be changed if we change ReactPPX4Support:

// as-is
let createElementWithKey = (~key=?, createElement, component, props) =>
  createElement(component, addKeyProp(~key?, props))

// to-be
let createElementWithKey = (~key=?, component, props) =>
  React.createElement(component, addKeyProp(~key?, props))

I think there are tradeoffs about adding rescript-react into the compiler, but not sure what the tradeoffs are clearly now.

[cristianoc]

Why is it passing React.createElement? That makes things worse (strong coupling via passing a function as argument) than before in my view. Is there a way to avoid it?

Basically wondering why createElement is not also defined here? It's just an external.

[mununki]

Basically wondering why createElement is not also defined here? It's just an external.

rescript-lang/rescript-react#76 (comment)

So my suggestion would be to move addKeyProp, createElementWithKey and createElementVariadicWithKey out of @rescript/react and into a new "PPX support module" in the compiler repo. Which means that we'd also need to duplicate the corresponding React.createElement bindings there. But that should be fine.

I missed the last part. I'll add it createElement binding into ReactPPX4Support

[mununki]

6345bdf
I've added the React binding for helper functions, and see that no passing function. And I've also see that it works fine in my large project.

[cknitt]

Looks great! Now the PPX and its helper functions are completely decoupled from the React bindings in @rescript/react. 👍🎉

[cristianoc]

Looks great!

[mununki]

I guess it shows ReactPPX4Support doesn't have dependency to Jsx. I think this causes the CI error.
Not sure how to fix it.

[mununki]

I'm gonna try fix this part in ninja.js

  var jsPrefixSourceFiles = othersDirFiles.filter(
    x =>
      x.startsWith("js") && // maybe? (x.startsWith("js") || x.startsWith("reactPPX4Support"))
      (x.endsWith(".ml") ||
        x.endsWith(".mli") ||
        x.endsWith(".res") ||
        x.endsWith(".resi")) &&
      !x.includes(".cppo") &&
      !x.includes(".pp") &&
      !x.includes("#") &&
      x !== "js.ml"
  );

[mununki]

Still wondering why the ci on mac-arm fails only.

[cristianoc]

Indeed the CI failure needs addressing.
It could be: that there can't be dependencies between things called js... and others.
Doing more sequential less parallel build would probably always fail.
Or something like that.

[cristianoc]

Sorry let me clarify: sequential vs parallel is just an explanation. It's not a suggestion to change the build.

The js... vs other names is a hypothesis. A possible reason why things don't work.

[cristianoc]

Indeed it looks like the 2 groups of others/ files, those starting with js, and those not starting with js, are built in parallel.

[mununki]

Indeed. I tried to fix with 5368e95 It seems not.

How about rewriting the ReactPPX4Support in ml? Would it be a temporary fix?

[mununki]

I guess that jsx.ml, jsxDOM.ml, etc. those were okay because their name starts with js.

[cristianoc]

That's not related to ml vs res

It's js... vs other names

[cristianoc]

I guess that jsx.ml, jsxDOM.ml, etc. those were okay because their name starts with js.

Yes that's the reason

[mununki]

How about this? Renaming reactPPX4Support to jsxPPX4Support to avoid the build issue. We can revisit to fix it after release 10.1 I think zero cost binding is more benefit we can get.

[cristianoc]

Looks good to me!

[mununki]

I've ran a test on the company project, it works nice!

[mununki]

Rebased to 10.1_release

[DZakh]

I'm migrating to JSX V4 and noticed JsxPPXReactSupport.createElementWithKey in the output. I wonder why it's not transformed to the runtime-free React.createElement as it used to be. Is there a reason? @mununki @cknitt @cristianoc

https://rescript-lang.org/try?version=v10.1.2&code=ALBWGcA8GEHsDsBmBLA5gCgN4DcCmAncZBALgAIBmAXwEoAoOgW1gBMBXAG1zICFZIAahTIBeMpjpkywfLgCGAYwAuAOgWxGABwS54SyWS5KyjOQGtuY9DVEA+cQakAeFsmz3MAJXnKV4JfjI8BgARIiwsCE0VGROAPSu7gZUdCl0AKT4cgDu6CFxcWQAKgAWyODkUXRGZLgcQqKxfILCFgCeIiEAjCFkcbYMdCAQMAgoGDgERKRkACwANCasuJUKHHLgRAohtAzM7Fy8-I0SUjI+qupaOnoGNaYWjdZ2DlLOiR7eiqr+gcF54Ui0ViCTcAykKTSmRyeQKZAAkvB-PIWGRYIgyEoyhUyFUanUBLNGk5mmR2p0en0BkA

[mununki]

Good question. There was a discussion about adding this helper function rescript-lang/syntax#693 (comment)
The TL;DR is that we don't want to expose the key to the user returned element, but at runtime we need to use it with the react api.
You can check the actual implementation of it. https://github.com/rescript-lang/rescript-compiler/blob/8de6c09fd1b0a4c6dcc667c3a4b6cbcebffe30c4/jscomp/others/jsxPPXReactSupportU.res#L4

[cknitt]

This is not an issue when using the new JSX runtime ("mode": "automatic") though.