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

Fix1736 #1745

Merged
merged 2 commits into from
Jan 18, 2018
Merged

Fix1736 #1745

merged 2 commits into from
Jan 18, 2018

Conversation

IwanKaramazow
Copy link
Contributor

@IwanKaramazow IwanKaramazow commented Jan 11, 2018

Fixes #1736

This PR adds printing of braces around JSX as JSX-arguments:

<Description term={<Text text="Age" />}> ... </Description>;
<Foo bar={<Baz />} />

Error messages for ambiguous cases are also provided:
captura de pantalla 2018-01-12 a las 21 35 02
captura de pantalla 2018-01-12 a las 21 35 57

@strega-nil
Copy link

This seems unfortunate. Could we make it context dependent instead?

@jordwalke
Copy link
Member

jordwalke commented Jan 12, 2018

Great! So, if we convert some code from OCaml to Reason that uses />>, how is it printed? I think we need a way to express the />>identifier with some test cases showing that it is possible. Perhaps you could have a special tokenizer rule for \/>> which parses into />>?

@IwanKaramazow
Copy link
Contributor Author

Per discussion with @chenglou, braces are printed around JSX as jsx-arguments:

<Description term={<Text text="Age" />}> ... </Description>;
<Foo bar={<Baz />} />

The infix operators are preserved this way, also added tests, and it's a bit more familiar for JS-developers.
Error messages for ambiguous cases are also provided:
captura de pantalla 2018-01-12 a las 21 35 02
captura de pantalla 2018-01-12 a las 21 35 57

@thangngoc89
Copy link
Contributor

Why don’t we use normal () braces? Those curly braces look ugly

@chenglou
Copy link
Member

@thangngoc89 mostly for familiarity with existing JSX, and to reduce the amount of parentheses.

@jordwalke
Copy link
Member

jordwalke commented Jan 17, 2018

To reduce the number of parens, we could print them as { } in any other JSX cases that require parens. It needn't block this diff, but one more "Good First Task" if anyone's interested.

@@ -2452,6 +2452,26 @@ let is_direct_pattern x = x.ppat_attributes = [] && match x.ppat_desc with
| Ppat_construct ( {txt= Lident"()"}, None) -> true
| _ -> false

let isJSXComponent loc args =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth pulling out JSX logic into its own module in the future.

@jordwalke jordwalke merged commit 765d47b into reasonml:master Jan 18, 2018
@jordwalke
Copy link
Member

Thank you again! Merged.

jordwalke pushed a commit that referenced this pull request Jan 21, 2018
* Allow parsing of <Description term=<Text text="Age" />>

* print jsx in jsx parameters with braces + extra tests
jordwalke pushed a commit that referenced this pull request Jan 23, 2018
* Allow parsing of <Description term=<Text text="Age" />>

* print jsx in jsx parameters with braces + extra tests
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.

6 participants