Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JSX string literal #1910

Closed
wants to merge 3 commits into from
Closed

Conversation

cristianoc
Copy link
Contributor

@cristianoc cristianoc commented Apr 26, 2018

Special case string literals in JSX so ReasonReact.string is added automatically.

Cristiano Calcagno added 3 commits April 25, 2018 16:39
Special case string literals in JSX so ReasonReact.stringToElement is added automatically.

TODO: use ReasonReact.string when the shorter function name will be added.
@cristianoc cristianoc changed the title Jsx string literal JSX string literal Apr 26, 2018
@IwanKaramazow
Copy link
Contributor

This raises another natural question, what happens with floats, ints, lists and arrays?
Since their "type" can be determined at lex-time and compiled with a ppx, do we still want the conversion for those others?

@wegry
Copy link

wegry commented May 2, 2018

@IwanKaramazow wouldn't it make sense that if we aren't sure about conversions, we stick with vanilla React's behavior, i.e. only strings are valid and not floats, ints, list, and arrays?

edit: Arrays of elements are valid children.

@rickyvetter
Copy link
Contributor

rickyvetter commented May 24, 2018

There are some pretty serious problems that come with this because ppx doesn't have access to the type information. It doesn't allow strings to be lifted outside of the ppx in a typesafe manner which will almost certainly result in issues with refactoring and some really confusing bugs for newcomers.

As an example:

<div> "foo" </div>

will typecheck and

let foo = "foo";
<div> foo </div>

will not.

@gilbert
Copy link

gilbert commented Jun 1, 2018

I think the string literal lifting issue is a small price to pay and should not be considered a deal breaker.

@rickyvetter
Copy link
Contributor

@gilbert it's not just lifting that will cause issues:

<div> {"Hello, " ++ name} </div>

won't typecheck and

<div> {localize("Title of the Page")} </div>

won't typecheck unless localize only works as a React element. You'll need a different function to operate outside of React.

If everyone were very familiar with these nuances then sure, I think this would be fine for convenience. What I'm worried about is how to explain this differing behavior to people who aren't aware of the nuances.

@gilbert
Copy link

gilbert commented Jun 1, 2018

I see, thanks for showing the other issues.

@kujon
Copy link

kujon commented Jun 6, 2018

It doesn't allow strings to be lifted outside of the ppx in a typesafe manner which will almost certainly result in issues with refactoring and some really confusing bugs for newcomers.

TypeScript has got a very similar behaviour when using type guards. It's the most annoying thing in the world, it makes you re-factor code only because the compiler doesn't understand.

Let's not go there with Reason please.

@rickyvetter
Copy link
Contributor

As an alternative I'm curious about how people feel about React Native's implementation with Text. We could expose a new <Text> </Text> element which takes a string as it's child and renders as you would expect. If we did that then we would probably remove ReasonReact.string and ask that strings are always placed into Text elements.

@kennetpostigo
Copy link
Contributor

kennetpostigo commented Jun 7, 2018

@rickyvetter I wonder how that would work for people using bs-react-native? They could probably alias the text from that library?

@rickyvetter
Copy link
Contributor

Yeah if this was a path we wanted to explore we'd work with the bs-react-native folks to make sure it would work for them.

@texastoland
Copy link

texastoland commented Jul 16, 2018

A text element makes sense. Alternatively why not just coerce {j|hello, $interpolation|j} (js too) to reactElement? That seems less intrusive and a newcomer likely also wants interpolation.

@facebook-github-bot
Copy link

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants