feat: automatic conversion of constants in JSX children to React.element

[anmonteiro]

No description provided.

[jchavarri]

Maybe this is early feedback as it's still a draft, but I am not pumped about the proposal. It doesn't follow the principle of least surprise because now the string literals will be interpreted differently by the PPX based on where they appear on the AST. I'm not sure it's a good tradeoff for the sake of saving a few keystrokes, or making ReasonReact component be more like JS ones, if those are the goals.

Also, I think it's safe to say we have a very limited idea at the moment about the impact and the amount of edge cases that will originate from this change, so if we decided to move forward, maybe we could put the new behavior under some experimental flag in the ppx so that people can opt-in to try it.

[jchavarri]

Below are mostly the points that Ricky already shared in reasonml/reason#1910, but I think it's good to have them here for completion.

---

This introduces a new "region" where things behave differently. E.g. this compiles:

module Foo = {
  [@react.component]
  let make = () => {
    <div> "a" </div>;
  };
};

but this does not:

module Foo = {
  [@react.component]
  let make = () => {
    let a = "a";
    <div> a </div>;
  };
};

I am not sure how I feel about these "regions", it reminds me a bit of useEffect and the dependencies, which is probably no bueno 😅

[jchavarri]

What happens in the following cases?

• Child is a variant or polyvar with a string payload
• Child is a quoted string
• Child is a tuple of strings
• Child is a function application with a string param
• Child is an extension with an string payload, e.g. [%intl "foo"], we use these a lot at Ahrefs. I particularly wonder if PPX ordering could have any impact, e.g. if the ppx processing intl generates a string literal, would the behavior change if it runs before or after reasonreact ppx?

[davesnx]

This only checks the Pconst kinds and applies the right React.string/int/float/char.

For all your cases, the type-checker will break. I would like to see the quality of the error in such cases

[jchavarri]

For all your cases, the type-checker will break.

I think the quoted string would pass because it's also Pconst_string. But it should prob be tested as well. Actually it might be interesting to test the whole thing (including type checking) and not only the syntactic transformation.

[anmonteiro]

yeah, if we move ahead with this, proper testing is required. I just included a simple cram syntactical example to show case what the PPX desugars to.

[anmonteiro]

It doesn't follow the principle of least surprise because now the string literals will be interpreted differently by the PPX based on where they appear on the AST. I'm not sure it's a good tradeoff for the sake of saving a few keystrokes, or making ReasonReact component be more like JS ones, if those are the goals.

Indeed these are the tradeoffs we knew we'd face when we first thought about this proposal. As you mentioned, it's also the main concern highlighted in reasonml/reason#1910.

Let me try to offer a perspective that I don't think has been shared yet:

• JSX (and Reason's JSX, by extension) is, effectively, a DSL (domain-specific language). Or it can be made to be a DSL through PPX interpretation.
• It can probably be explained that everything captured under a JSX element's children treats literals in a special way, and that they're inferred syntactically rather than at type-checking time

Now, probably the reasonable thing to do would be to gather some data of the ratio between calling React.string on literals vs. on bindings. And then assess whether that ratio is sufficiently disparate to justify implementing this proposal.

How else are you thinking about the tradeoffs here? I'd be curious to learn what tradeoffs I'm missing.

[jchavarri]

Now, probably the reasonable thing to do would be to gather some data of the ratio between calling React.string on literals vs. on bindings. And then assess whether that ratio is sufficiently disparate to justify implementing this proposal.

I can take this PR for a ride on Ahrefs codebase and see what % of usages of React.string or other converters can be removed, this also might help us surface some unknown problems. There are ~9k usages of React.string in the codebase.

But it'd have to be after the migration to React 18 / latest reason-react, which is currently ongoing.

[anmonteiro]

This again proves to be not sufficient for the effects that we actually needed, which is removing most (likely all React.array etc) conversions.