Skip to content

allow hyphens in JSX tag names and props #6609

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

Merged
merged 4 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

#### :rocket: New Feature

- Add support for array spread. https://github.com/rescript-lang/rescript-compiler/pull/6608
- Support import attributes (https://github.com/tc39/proposal-import-attributes) in `@module()`. https://github.com/rescript-lang/rescript-compiler/pull/6599
- allow hyphens in jsx tag names (e.g. `<mj-column>`). https://github.com/rescript-lang/rescript-compiler/pull/6609

#### :bug: Bug Fix

Expand All @@ -36,7 +38,6 @@
- Experimental support of tagged template literals, e.g. ```sql`select * from ${table}```. https://github.com/rescript-lang/rescript-compiler/pull/6250
- Experimental support for generic/custom JSX transforms. https://github.com/rescript-lang/rescript-compiler/pull/6565
- `dict` is now a builtin type. https://github.com/rescript-lang/rescript-compiler/pull/6590
- Add support for array spread. https://github.com/rescript-lang/rescript-compiler/pull/6608

#### :bug: Bug Fix

Expand Down
26 changes: 15 additions & 11 deletions jscomp/syntax/src/res_core.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2617,10 +2617,11 @@ and parseJsxOpeningOrSelfClosingElement ~startPos p =
| GreaterThan -> (
(* <foo a=b> bar </foo> *)
let childrenStartPos = p.Parser.startPos in
Scanner.setJsxMode p.scanner;
Parser.next p;
let spread, children = parseJsxChildren p in
let childrenEndPos = p.Parser.startPos in
Scanner.popMode p.scanner Jsx;
Scanner.setJsxMode p.scanner;
Comment on lines +2623 to +2624
Copy link
Member Author

Choose a reason for hiding this comment

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

by using popMode before setJsxMode we make sure we don't add too many times the Jsx mode to the scanner mode list.

I also made sure we were calling setJsxMode at the right place, which was not really the case before and led to invalid results (allowing hyphens in contexts that should not have been in Jsx mode).

let () =
match p.token with
| LessThanSlash -> Parser.next p
Expand Down Expand Up @@ -2685,6 +2686,8 @@ and parseJsxOpeningOrSelfClosingElement ~startPos p =
* jsx-children ::= primary-expr* * => 0 or more
*)
and parseJsx p =
Scanner.popMode p.scanner Jsx;
Scanner.setJsxMode p.Parser.scanner;
Comment on lines +2689 to +2690
Copy link
Member Author

Choose a reason for hiding this comment

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

same here, making sure we call setJsxMode when calling parseJsx

Parser.leaveBreadcrumb p Grammar.Jsx;
let startPos = p.Parser.startPos in
Parser.expect LessThan p;
Expand All @@ -2696,6 +2699,7 @@ and parseJsx p =
parseJsxFragment p
| _ -> parseJsxName p
in
Scanner.popMode p.scanner Jsx;
Parser.eatBreadcrumb p;
{jsxExpr with pexp_attributes = [jsxAttr]}

Expand All @@ -2706,12 +2710,12 @@ and parseJsx p =
*)
and parseJsxFragment p =
let childrenStartPos = p.Parser.startPos in
Scanner.setJsxMode p.scanner;
Parser.expect GreaterThan p;
let _spread, children = parseJsxChildren p in
let childrenEndPos = p.Parser.startPos in
Parser.expect LessThanSlash p;
Parser.expect GreaterThan p;
Scanner.popMode p.scanner Jsx;
let loc = mkLoc childrenStartPos childrenEndPos in
makeListExpression loc children None

Expand Down Expand Up @@ -2743,6 +2747,7 @@ and parseJsxProp p =
Parser.next p;
(* no punning *)
let optional = Parser.optional p Question in
Scanner.popMode p.scanner Jsx;
let attrExpr =
let e = parsePrimaryExpr ~operand:(parseAtomicExpr p) p in
{e with pexp_attributes = propLocAttr :: e.pexp_attributes}
Expand All @@ -2765,6 +2770,7 @@ and parseJsxProp p =
Parser.next p;
match p.Parser.token with
| DotDotDot -> (
Scanner.popMode p.scanner Jsx;
Parser.next p;
let loc = mkLoc p.Parser.startPos p.prevEndPos in
let propLocAttr =
Expand All @@ -2790,9 +2796,7 @@ and parseJsxProps p =
and parseJsxChildren p =
let rec loop p children =
match p.Parser.token with
| Token.Eof | LessThanSlash ->
Scanner.popMode p.scanner Jsx;
List.rev children
| Token.Eof | LessThanSlash -> children
| LessThan ->
(* Imagine: <div> <Navbar /> <
* is `<` the start of a jsx-child? <div …
Expand All @@ -2808,23 +2812,23 @@ and parseJsxChildren p =
else
(* LessThanSlash *)
let () = p.token <- token in
let () = Scanner.popMode p.scanner Jsx in
List.rev children
children
| token when Grammar.isJsxChildStart token ->
let () = Scanner.popMode p.scanner Jsx in
let child =
parsePrimaryExpr ~operand:(parseAtomicExpr p) ~noCall:true p
in
loop p (child :: children)
| _ ->
Scanner.popMode p.scanner Jsx;
List.rev children
| _ -> children
in
match p.Parser.token with
| DotDotDot ->
Parser.next p;
(true, [parsePrimaryExpr ~operand:(parseAtomicExpr p) ~noCall:true p])
| _ -> (false, loop p [])
| _ ->
let children = List.rev (loop p []) in
Scanner.popMode p.scanner Jsx;
(false, children)
Comment on lines +2828 to +2831
Copy link
Member Author

Choose a reason for hiding this comment

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

here I simplified the loop function by applying recurring processing to the result instead of doing it for every branch.

I also fixed the calls to pop/set jsx mode.


and parseBracedOrRecordExpr p =
let startPos = p.Parser.startPos in
Expand Down
10 changes: 6 additions & 4 deletions jscomp/syntax/src/res_printer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ let printLongident = function

type identifierStyle = ExoticIdent | NormalIdent

let classifyIdentContent ?(allowUident = false) txt =
let classifyIdentContent ?(allowUident = false) ?(allowHyphen = false) txt =
if Token.isKeywordTxt txt then ExoticIdent
else
let len = String.length txt in
Expand All @@ -431,16 +431,18 @@ let classifyIdentContent ?(allowUident = false) txt =
match String.unsafe_get txt i with
| 'A' .. 'Z' when allowUident -> loop (i + 1)
| 'a' .. 'z' | '_' -> loop (i + 1)
| '-' when allowHyphen -> loop (i + 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

here we make identifiers with hyphen regular identifier instead of exotic ones that need to be printed inside double quotes

| _ -> ExoticIdent
else
match String.unsafe_get txt i with
| 'A' .. 'Z' | 'a' .. 'z' | '0' .. '9' | '\'' | '_' -> loop (i + 1)
| '-' when allowHyphen -> loop (i + 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

same here

| _ -> ExoticIdent
in
loop 0

let printIdentLike ?allowUident txt =
match classifyIdentContent ?allowUident txt with
let printIdentLike ?allowUident ?allowHyphen txt =
match classifyIdentContent ?allowUident ?allowHyphen txt with
| ExoticIdent -> Doc.concat [Doc.text "\\\""; Doc.text txt; Doc.text "\""]
| NormalIdent -> Doc.text txt

Expand Down Expand Up @@ -4462,7 +4464,7 @@ and printJsxProp ~state arg cmtTbl =
* Navabar.createElement -> Navbar
* Staff.Users.createElement -> Staff.Users *)
and printJsxName {txt = lident} =
let printIdent = printIdentLike ~allowUident:true in
let printIdent = printIdentLike ~allowUident:true ~allowHyphen:true in
let rec flatten acc lident =
match lident with
| Longident.Lident txt -> printIdent txt :: acc
Expand Down
7 changes: 5 additions & 2 deletions jscomp/syntax/src/res_scanner.ml
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,11 @@ let digitValue ch =
let scanIdentifier scanner =
let startOff = scanner.offset in
let rec skipGoodChars scanner =
match scanner.ch with
| 'A' .. 'Z' | 'a' .. 'z' | '0' .. '9' | '_' | '\'' ->
match (scanner.ch, inJsxMode scanner) with
| ('A' .. 'Z' | 'a' .. 'z' | '0' .. '9' | '_' | '\''), false ->
next scanner;
skipGoodChars scanner
| ('A' .. 'Z' | 'a' .. 'z' | '0' .. '9' | '_' | '\'' | '-'), true ->
Copy link
Member Author

Choose a reason for hiding this comment

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

here we allow - as a valid identifier in Jsx mode.

next scanner;
skipGoodChars scanner
| _ -> ()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@

Syntax error!
tests/parsing/errors/expressions/jsx.res:1:12

1 │ let x = <di-v />
2 │ let x = <Unclosed >;
3 │ let x = <Foo.Bar></Free.Will>;

I'm not sure what to parse here when looking at "-".


Comment on lines -2 to -11
Copy link
Member Author

Choose a reason for hiding this comment

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

<di-v is now a valid tag name for lowercase jsx tag, so the error got removed

Syntax error!
tests/parsing/errors/expressions/jsx.res:2:20

Expand Down Expand Up @@ -66,7 +56,7 @@

I'm not sure what to parse here when looking at ".".

let x = ((di ~children:[] ())[@JSX ]) - (v / ([%rescript.exprhole ]))
let x = ((di-v ~children:[] ())[@JSX ])
Comment on lines -69 to +59
Copy link
Member Author

Choose a reason for hiding this comment

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

a valid tag now instead of a exprhole

let x = ((Unclosed.createElement ~children:[] ())[@JSX ])
let x =
((Foo.Bar.createElement ~children:[] ())[@JSX ]) > ([%rescript.exprhole ])
Expand Down
12 changes: 6 additions & 6 deletions jscomp/syntax/tests/printer/expr/expected/jsx.res.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ let x = <Foo.Bar className="container" />
let x = <Foo.Bar.Baz className="container" />
let x = <Foo.bar className="container" />
let x = <Foo.baz className="multiline" />
let x = <\"custom-tag" className="container" />
let x = <Foo.\"custom-tag" className="container" />
let x = <custom-tag className="container" />
let x = <Foo.custom-tag className="container" />
Comment on lines -7 to +8
Copy link
Member Author

Choose a reason for hiding this comment

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

<custom-tag is now allowed and should not be printed as <\"custom-tag" exotic identifier, hence the update


// https://github.com/rescript-lang/syntax/issues/570
let x =
Expand Down Expand Up @@ -38,15 +38,15 @@ let x =
{b}
</A>
let x =
<\"custom-tag" className="container">
<custom-tag className="container">
{a}
<B />
</\"custom-tag">
</custom-tag>
let x =
<Foo.\"custom-tag" className="container">
<Foo.custom-tag className="container">
{a}
<B />
</Foo.\"custom-tag">
</Foo.custom-tag>

let x = <div className="container" className2="container2" className3="container3" onClick />

Expand Down
8 changes: 4 additions & 4 deletions jscomp/syntax/tests/printer/expr/jsx.res
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,17 @@ let x = <Foo.bar className="container" />
let x = <Foo.baz
className="multiline"
/>
let x = <\"custom-tag" className="container" />
let x = <Foo.\"custom-tag" className="container" />
let x = <custom-tag className="container" />
let x = <Foo.custom-tag className="container" />

// https://github.com/rescript-lang/syntax/issues/570
let x = <A> <B> <C> <D /> <E /> </C> <F> <G /> <H /> </F> </B> </A>
let x = <A> {children} <B/> </A>
let x = <A> <B/> {children} </A>
let x = <A> {a} </A>
let x = <A> {a} {b} </A>
let x = <\"custom-tag" className="container" > {a} <B/> </\"custom-tag">
let x = <Foo.\"custom-tag" className="container" > {a} <B/> </Foo.\"custom-tag">
let x = <custom-tag className="container" > {a} <B/> </custom-tag>
let x = <Foo.custom-tag className="container" > {a} <B/> </Foo.custom-tag>

let x =
<div
Expand Down