reasonml / reason-react

Reason bindings for ReactJS
https://reasonml.github.io/reason-react/
MIT License
3.25k stars 347 forks source link

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

Open anmonteiro opened 10 months ago

anmonteiro commented 10 months ago

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 https://github.com/reasonml/reason/pull/1910.

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

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 commented 10 months ago

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.