-
Notifications
You must be signed in to change notification settings - Fork 471
Remove support of children spread inside JSX #7869
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
Remove support of children spread inside JSX #7869
Conversation
Co-authored-by: tsnobip <2479216+tsnobip@users.noreply.github.com>
d7ba92c to
4b8a0c5
Compare
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
|
Looks good at a glance. Can we add a test to check which parsing error we now get for |
compiler/ml/parsetree.ml
Outdated
| and jsx_children = | ||
| | JSXChildrenSpreading of expression | ||
| | JSXChildrenItems of expression list | ||
| and jsx_children = JSXChildrenItems of expression list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed now as well, right? Should probably do that in the same PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zth let me know if you additional comments!
@nojaf that's a good point, unfortunately the error is indeed quite cryptic! |
compiler/syntax/src/res_core.ml
Outdated
| Parsetree.JSXChildrenItems children | ||
| in | ||
| children | ||
| List.rev (loop p []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tsnobip inside the loop you probably want to match ... and give a better parse error.
|
|
||
| Syntax error! | ||
| syntax_tests/data/parsing/errors/expressions/jsx.res:18:15-17 | ||
|
|
||
| 16 │ | ||
| 17 │ // spread children | ||
| 18 │ let x = <div> ...c </div> | ||
|
|
||
| Spreading JSX children is no longer supported. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nojaf is it better like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, does wrapping it {...c} do anything?
Syntax-wise probably that is going to be valid.
Would surprise me if that type-checks though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nojaf no, it would not type-checks.
But that's actually why I wanted first to merge this PR, wrapping will do ...{c} and the compiler will complain that spreading children is no longer supported!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
|
Why was this removed? And why is the only description of this change a useless AI summary that says nothing about the rationale or possible consequences of such a change? Have we completely abandoned any attempt at human-human interaction now? |
|
If I recall correctly, the use-case was very limited that it didn't seem worth keeping. https://rescript-lang.org/docs/manual/v9.0.0/jsx#children-spread The output in v11 seems to be the same: https://rescript-lang.org/try?version=v11.1.4&module=esmodule&code=DYUwLgBAnhC8EB4BGAnCB6AfAKFJAXnIgCYCWAbptIumZbuBAB5EJ1UB0XMCtFOQA so that lead us to believe this was no longer relevant. Were you using this? |
|
Thanks @nojaf. Yes, I've been using it quite extensively, But it does indeed seem to be unnecessary now, which makes this change a lot less invasive than I thought it was. |
|
And sorry for lashing out. I have severe AI fatigue and am seriously considering quitting software development altogether because of it. |
|
No worries, I can relate to that 😊. Another reason to drop this, is that we have some post v12 ideas to bring JSX syntax closer to the JavaScript JSX. And not having to deal with this would benefit that. |
This PR removes support for JSX children spread syntax while preserving props spread functionality.
What was removed
Children spread syntax like this is no longer supported:
What still works
Props spread continues to work as before:
Normal JSX children also continue to work:
Implementation details
The changes remove the
JSXChildrenSpreadingvariant from thejsx_childrentype and update all related code:parse_jsx_childrento no longer recognize...syntax in children contextJSXChildrenSpreading of expressionfromjsx_childrentypeThe implementation is minimal and surgical - only the children spread functionality is removed while all other JSX features remain unchanged.
Files changed
parsetree.mlres_core.mland printer inres_printer.mlcompiler/ml/compiler/syntax/src/This change makes the JSX syntax more consistent by having spread syntax only available in props context.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.