allow hyphens in JSX tag names and props #6609
Conversation
There was a problem hiding this comment.
This is great! I'm unfamiliar with the parser mode etc where most of the changes are, so I'm going to defer reviewing that part to @cristianoc who I believe knows more about that. Outside of that this looks like it does just what we want, great work!
|
not familiar with that part of the parser either, perhaps a little explanation of the changes would be useful, unless the changes are obviously correct |
There was a problem hiding this comment.
I added a few comments to the PR to make it more understandable @cristianoc
| | ('A' .. 'Z' | 'a' .. 'z' | '0' .. '9' | '_' | '\''), false -> | ||
| next scanner; | ||
| skipGoodChars scanner | ||
| | ('A' .. 'Z' | 'a' .. 'z' | '0' .. '9' | '_' | '\'' | '-'), true -> |
There was a problem hiding this comment.
here we allow - as a valid identifier in Jsx mode.
| @@ -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) | |||
There was a problem hiding this comment.
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) |
| Scanner.popMode p.scanner Jsx; | ||
| Scanner.setJsxMode p.scanner; |
There was a problem hiding this comment.
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).
| Scanner.popMode p.scanner Jsx; | ||
| Scanner.setJsxMode p.Parser.scanner; |
There was a problem hiding this comment.
same here, making sure we call setJsxMode when calling parseJsx
| | _ -> | ||
| let children = List.rev (loop p []) in | ||
| Scanner.popMode p.scanner Jsx; | ||
| (false, children) |
There was a problem hiding this comment.
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.
| 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" /> |
There was a problem hiding this comment.
<custom-tag is now allowed and should not be printed as <\"custom-tag" exotic identifier, hence the update
| 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 "-". | ||
|
|
||
|
|
There was a problem hiding this comment.
<di-v is now a valid tag name for lowercase jsx tag, so the error got removed
| let x = ((di ~children:[] ())[@JSX ]) - (v / ([%rescript.exprhole ])) | ||
| let x = ((di-v ~children:[] ())[@JSX ]) |
There was a problem hiding this comment.
a valid tag now instead of a exprhole
|
@tsnobip 🎉 would you add some docs too? |
sure, what would you like me to document? Should I add what characters are allowed for uncapitalized jsx tags here https://rescript-lang.org/docs/manual/latest/jsx#uncapitalized? |
|
A sub section like "Spread Props" perhaps? Just a quick note that hyphens are allowed in this particular case and can be used with web components etc. |
feature brought by rescript-lang/rescript#6609
@zth done here rescript-lang/rescript-lang.org#812 |
feature brought by rescript-lang/rescript#6609
remaining:- [ ] hyphens in prop namesEdit: We don't actually need to allow hyphens in prop names since we can use
@asfor them.The current situation where scanner.mode is a list is not ideal, you end up pushing multiple times to this list, you never know what's the mode once you popped up, etc. I tried using GADT so you could restrict some functions to a given mode, but it doesn't play well with the rest of the interface (mutable fields everywhere). Meanwhile it could maybe just be replaced with just a value instead of a list. What's is even Diamond mode by the way?