Add support for functors in JSX tags #1927

OhadRau · 2018-05-15T03:25:14Z

Fixes #1924

This PR enables the use of functors in JSX tags. It may also alter some edge cases in the syntax (though these would not compile properly in the past) -- previously, <M./> would parse correctly but fail to bind to a module; in certain cases, this will still parse but it should result in an error earlier in the pipeline (indicating a syntax error). This PR allows for the following new syntax:

<F(X)/>;
<F(X)> </F>;
// <F(X)> </F(X)>; is not currently supported, but this would be an easy addition

The syntax is designed to make it clear that the functor application only occurs once, but again it is a relatively easy change to add support for (or even require) a matching end tag.

Internally, the transformation is done by creating a let module expression to temporarily bind the result of the applied functor and then calling the createElement function on that result. Some more info, as well as an example use-case, is show in #1924.

facebook-github-bot · 2018-05-15T03:25:30Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2018-05-15T03:32:44Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

cristianoc · 2018-05-15T03:39:05Z

Could you check what happens with the use case

<Foo.Bar/>

That's where the . character is expected to be used.
Perhaps there isn't a test for that use at the moment, and it should be added.

OhadRau · 2018-05-15T04:03:39Z

Yeah, that seems to still work. To clarify, it should only affect the case with trailing dots, since I'm basically allowing for an arbitrary Longident (rather than only the Lident and Ldot constructors) for the capitalized case. However, the <M.x/> case doesn't work with this change--is this something that's intended to work? (Update: I've pushed a commit that should fix that specific case hopefully)

cristianoc · 2018-05-15T11:45:32Z

src/reason-parser/reason_lexer.mll

@@ -532,7 +532,7 @@ rule token = parse
  | "[|" { LBRACKETBAR }
  | "[<" { LBRACKETLESS }
  | "[>" { LBRACKETGREATER }
-  | "<" uppercase_or_lowercase (identchar | '.') * {
+  | "<" (uppercase identchar* '.')* lowercase identchar* {


To double check: the main change here is uppercase_or_lowercase to lowercase, plus some . cleanup correct?

The cleanup can be applied to the two cases below too, using I guess
(uppercase identchar* '.')* uppercase_or_lowercase identchar*

Yes. The change to lowercase as the main pattern is to make it only parse lowercase tags (e.g. <div>) as LESSIDENT. The parser implements the uppercase (opening) tags as a different pattern without LESSIDENT. The . cleanup was specifically added to allow tags like <X.x>, but doesn't support <X.Y>. I could add this to the other cases, but they work simply by resetting the lexer to the beginning of the pattern, consuming only up to the < token. I assume that the current patterns will run slightly faster, and should work correctly at the parsing level. That said, it may make more sense to switch to (uppercase identchar* '.')* lowercase identchar* and (uppercase identchar* '.')* uppercase identchar* for the other patterns to make them look more similar and make the patterns more accurate.

cristianoc · 2018-05-15T11:46:47Z

formatTest/unit_tests/expected_output/jsx_functor.re

+  let createElement = X.createElement;
+};
+
+let body = <div> <F(X) /> </div>;


Are several arguments supported?

<F(X,Y) />

Yes, as well as full paths , e.g.(<G.F(X, Y)/>). Applying multiple separate functors, e.g. <G(X).F(Y, Z)/> doesn't work (the AST transformation required for that is a lot more complex).

I've tried to add a test for <F(X,Y) /> but I'm getting a syntax error on the file generated in actual_output. Could you try adding a multi argument example to the tests?

Hm, I think the syntax error is from the idempotent tests. When I test it with refmt it doesn't seem to print right. Just fixed it

OhadRau · 2018-05-17T02:01:54Z

Not sure if you got a notification from the last pushes, but I've fixed the pretty-printing for functors with multiple arguments. Let me know if there's anything else that might be an issue with this.

cristianoc · 2018-05-17T03:28:12Z

I wasn't sure what the plan was about tests. The PR currently adds empty files for jsx_functor.re.

OhadRau · 2018-05-17T04:38:42Z

Weird, I must've accidentally cleared those files before pushing. Should be fixed now.

cristianoc · 2018-05-17T05:32:06Z

Thanks!
From the CI, it looks like idempotent tests are failing with a syntax error

Idempotent Test: ./jsx_functor.re
File "/home/opam/project/formatTest/unit_tests/actual_output/./jsx_functor.re", line 36, characters 26-28:

OhadRau · 2018-05-17T05:49:14Z

I feel stupid cause I keep running make test and thinking it worked only to realize I pushed broken code lol. Should work now

cristianoc · 2018-05-17T06:00:35Z

src/reason-parser/reason_pprint_ast.ml

+         the tag, then it would be consistent with array spread:
+         [...list] evaluates to the thing as list.
+      *)
+      let rec extract_apps args = function


The compiler gives a non-exhaustive pattern matching warning here.

cristianoc · 2018-05-17T06:01:44Z

src/reason-parser/reason_pprint_ast.ml

+      if hasLabelledChildrenLiteral && hasSingleNonLabelledUnitAndIsAtTheEnd l then
+        if List.length (Longident.flatten loc.txt) > 1 then
+          if Longident.last loc.txt = "createElement" then
+            let ftor::args = extract_apps [] app in


non-exhaustive pattern matching here too

perhaps the code can be refactored a bit

cristianoc · 2018-05-17T18:43:55Z

Great!
This looks good to me. Is it ready to merge?

OhadRau · 2018-05-17T18:51:49Z

Yep, seems like it's all done. Thank you!

jordwalke · 2018-05-17T23:24:52Z

Do you think it might surprise some people that each time you use the JSX functor application, it will instantiate a new module each time?
<X(Y)> is not the same resulting module as <X(Y)> used a second time, although their types are identical (thank you module aliases).

So if the functor application has a side effect it would occur twice, or if it has a reference cell, it will not be shared. Do you have thoughts on that?

OhadRau · 2018-05-18T03:41:43Z

I think anyone who's used OCaml/Reason a lot would assume it based on syntax, because each instance of X(Y) isn't the module. Also, I think intuitively it looks like a function application, which would hopefully be enough to get the user to realize the side effects are occurring each time. Maybe this is something that could be addressed in the docs? I doubt anyone but long-time users of OCaml/Reason would try this out, since it's not something that exists in the "real" JSX. Because of that, I think >90% of Reason users will only be introduced to this syntax by the docs, meaning that we can specifically mention it with a big warning. The reference cell might be something to be concerned with, but as far as I know, there's no way of accessing anything other than createElement without a PPX and hopefully that is enough to push people towards reducing or eliminating side effects of module instantiation.

OhadRau added 3 commits May 14, 2018 17:38

Add functor support to JSX tags

608d8f7

Add tests for functors in JSX syntax

3773738

Add pretty-printing and change functor naming

5f6adb9

facebook-github-bot added CLA Signed labels May 15, 2018

Fix module-member access in JSX tags

59ded6f

cristianoc reviewed May 15, 2018

View reviewed changes

OhadRau added 2 commits May 15, 2018 17:23

Fix pretty printing for multi-param functors in JSX tags

71a4f2a

Update broken test case for jsx-functor

76a511e

Update JSX-functor tests

5daa99b

Fix pretty printing of close tags for JSX functors

fe0a291

cristianoc reviewed May 17, 2018

View reviewed changes

Remove warnings in pretty-printer for JSX functors

3ed1903

cristianoc merged commit 9bf799d into reasonml:master May 17, 2018

Drup mentioned this pull request Jan 31, 2020

Better handling of longidents in JSX #2541

Merged

OhadRau mentioned this pull request Jun 4, 2020

feat(components): add a Markdown component revery-ui/revery#886

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for functors in JSX tags #1927

Add support for functors in JSX tags #1927

OhadRau commented May 15, 2018

facebook-github-bot commented May 15, 2018

facebook-github-bot commented May 15, 2018

cristianoc commented May 15, 2018

OhadRau commented May 15, 2018 •

edited

Loading

cristianoc May 15, 2018

OhadRau May 15, 2018

cristianoc May 15, 2018

OhadRau May 15, 2018

cristianoc May 15, 2018

OhadRau May 15, 2018 •

edited

Loading

OhadRau commented May 17, 2018

cristianoc commented May 17, 2018

OhadRau commented May 17, 2018

cristianoc commented May 17, 2018

OhadRau commented May 17, 2018

cristianoc May 17, 2018

cristianoc May 17, 2018

cristianoc commented May 17, 2018

OhadRau commented May 17, 2018

jordwalke commented May 17, 2018 •

edited

Loading

OhadRau commented May 18, 2018

Add support for functors in JSX tags #1927

Add support for functors in JSX tags #1927

Conversation

OhadRau commented May 15, 2018

facebook-github-bot commented May 15, 2018

facebook-github-bot commented May 15, 2018

cristianoc commented May 15, 2018

OhadRau commented May 15, 2018 • edited Loading

cristianoc May 15, 2018

Choose a reason for hiding this comment

OhadRau May 15, 2018

Choose a reason for hiding this comment

cristianoc May 15, 2018

Choose a reason for hiding this comment

OhadRau May 15, 2018

Choose a reason for hiding this comment

cristianoc May 15, 2018

Choose a reason for hiding this comment

OhadRau May 15, 2018 • edited Loading

Choose a reason for hiding this comment

OhadRau commented May 17, 2018

cristianoc commented May 17, 2018

OhadRau commented May 17, 2018

cristianoc commented May 17, 2018

OhadRau commented May 17, 2018

cristianoc May 17, 2018

Choose a reason for hiding this comment

cristianoc May 17, 2018

Choose a reason for hiding this comment

cristianoc commented May 17, 2018

OhadRau commented May 17, 2018

jordwalke commented May 17, 2018 • edited Loading

OhadRau commented May 18, 2018

OhadRau commented May 15, 2018 •

edited

Loading

OhadRau May 15, 2018 •

edited

Loading

jordwalke commented May 17, 2018 •

edited

Loading