- Notifications
You must be signed in to change notification settings - Fork471
Wrap non-components JSX children in curly braces#7863
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
res<span> hello </span>should be printed asres<span>{hello}</span>andres<span> hello <span/></span>should be printed as ```res<span>{hello}<s...tsnobip commentedSep 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@copilot update the syntax tests accordingly and fix the wrapping of spread variables: <div>...element </div> should be wrapped like this: not like this: <div>...{element} </div> EDIT: Not necessary anymore, children spread inside JSX is no longer supported since#7869. |
... Fixed spread variables in JSX children to be wrapped in curly braces. The change ensures |
312375d toe6ebbb9Comparepkg-pr-newbot commentedSep 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
e6ebbb9 tod5cc8eaCompareIt is quite the turnoff that the PR description mentions code that doesn't compile:  The goal is to wrap Jsx.element type identifiers in braces? |
According to the PR description, letx= <span>hello </span> should be reformatted to letx= <span>{hello}</span> but instead it does keep the spaces: letx= <span> {hello} </span> This is not ideal, because in JavaScript JSX those spaces would actually go into the output: |
Yes only for Jsx.element for now, AI made a pretty terrible description of the PR, I actually restarted it from scratch, I should have maybe create another PR instead, to avoid this noise. |
Yes whitespace is a big issue if we start having string templates. |
Maybe wrap stuff in "" if there are spaces between words, but trim the outer spaces away. Could be similar to how polymorphic variants do it.
|
Uh oh!
There was an error while loading.Please reload this page.
This PR modifies the ReScript printer to wrap variables (identifiers) in curly braces when they appear as JSX children, making the output consistent with standard JSX conventions.
Problem
Previously, variables inside JSX children were printed without braces:
This is inconsistent with JSX standards where variables and spread expressions should be wrapped in braces to distinguish them from text content.
Solution
With this change, variables and constants are now wrapped in curly braces:
Implementation
The fix involves surgical changes to the
jsx_child_exprfunction incompiler/syntax/src/res_parens.ml:Pexp_ident _(variables/identifiers) from the pattern that returnedNothing, causing them to fall through to the default case that returnsParenthesized, which wraps them in braces.What Changes
Now wrapped in braces:
hello→{hello}someVar→{someVar}"hello"->{"hello"}obj.field->{obj.field}Some(x)->{Some(x)}%raw("eval()")->{%raw("eval()")}{"x": 1, "y": 2}->{{"x": 1, "y": 2}}Already had braces (no change):
{func()}stays{func()}{a + b}stays{a + b}** Remaining to support **
Testing
All existing syntax tests have been updated to reflect the new behavior. The changes are minimal and targeted, affecting only variables in JSX children while preserving existing behavior for all other expression types.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn moreCopilot coding agent tips in the docs.