rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

Wrap non-components JSX children in curly braces

Open Copilot opened this issue 3 months ago • 8 comments

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:

let hello = "world"
let c = "test"
let x = <span> hello </span>
let y = <span> hello <span/></span>
let z = <div>...c</div>

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:

let hello = "world"
let c = "test"
let x = <span>{hello}</span>
let y = <span>{hello}<span/></span>
let z = <div>{...c}</div>

Implementation

The fix involves surgical changes to the jsx_child_expr function in compiler/syntax/src/res_parens.ml:

  1. Variables: Removed Pexp_ident _ (variables/identifiers) from the pattern that returned Nothing, causing them to fall through to the default case that returns Parenthesized, which wraps them in braces.

What Changes

Now wrapped in braces:

  • Variables: hello{hello}
  • Identifiers: someVar{someVar}
  • constants: "hello" -> {"hello"}
  • field access: obj.field -> {obj.field}
  • constructors: Some(x) -> {Some(x)}
  • extensions: %raw("eval()") -> {%raw("eval()")}
  • records and objects: {"x": 1, "y": 2} -> {{"x": 1, "y": 2}}

Already had braces (no change):

  • Function calls: {func()} stays {func()}
  • Binary expressions: {a + b} stays {a + b}

** Remaining to support **

  • [ ] comments

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 more Copilot coding agent tips in the docs.

Copilot avatar Sep 09 '25 13:09 Copilot

@copilot update the syntax tests accordingly and fix the wrapping of spread variables:

<div> ...element </div>

should be wrapped like this:

<div> {...element} </div>

not like this:

<div> ...{element} </div>

EDIT: Not necessary anymore, children spread inside JSX is no longer supported since #7869.

tsnobip avatar Sep 09 '25 15:09 tsnobip

@copilot update the syntax tests accordingly and fix the wrapping of spread variables:

<div> ...element </div>

...

Fixed spread variables in JSX children to be wrapped in curly braces. The change ensures ...variable becomes {...variable} instead of remaining unwrapped. Updated syntax tests accordingly. Commit: db4b047

Copilot avatar Sep 09 '25 15:09 Copilot

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7863
@rescript/darwin-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-arm64@7863
@rescript/darwin-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/darwin-x64@7863
@rescript/linux-arm64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-arm64@7863
@rescript/linux-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/linux-x64@7863
@rescript/runtime

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/runtime@7863
@rescript/win32-x64

npm i https://pkg.pr.new/rescript-lang/rescript/@rescript/win32-x64@7863

commit: d5cc8ea

pkg-pr-new[bot] avatar Sep 12 '25 06:09 pkg-pr-new[bot]

It is quite the turnoff that the PR description mentions code that doesn't compile:

image

The goal is to wrap Jsx.element type identifiers in braces?

nojaf avatar Sep 12 '25 07:09 nojaf

According to the PR description,

let x = <span> hello </span>

should be reformatted to

let x = <span>{hello}</span>

but instead it does keep the spaces:

let x = <span> {hello} </span>

This is not ideal, because in JavaScript JSX those spaces would actually go into the output: https://babeljs.io/repl#?config_lz=N4IgZglgNgpgdgQwLYxALhAJxgBygOgCsBnADxABoQdtiYAXY9AbWZHgDdLR6FMBzBkwwATGGAQBXKIwoACOAHt6ciDDkBGDfKUq1AfSSKARpo2UQRkdJjCJUOlWOT-kUrfT1MkmAF8AuhRs2AgAxvTcWJJw9BAo6CBS9IpICLGhIAFBIMS8ggC0AEyRYqGKmGnlxABqMJjEEIpwCYUADIUAzPlaFjgQODBQEHAwAAqYijiKxAhQCQAWYQDWmf6BOYqSmKEwACoAngMJVjaZQA&code_lz=DYUwLgBAHhC8EB4DOAHAhgOwHwQN4AsRhgB7AX0QHpVMsAoIA&lineWrap=true&version=7.28.4

cknitt avatar Sep 12 '25 12:09 cknitt

It is quite the turnoff that the PR description mentions code that doesn't compile:

image

The goal is to wrap Jsx.element type identifiers in braces?

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.

tsnobip avatar Sep 12 '25 12:09 tsnobip

According to the PR description,

let x = <span> hello </span>

should be reformatted to

let x = <span>{hello}</span>

but instead it does keep the spaces:

let x = <span> {hello} </span>

This is not ideal, because in JavaScript JSX those spaces would actually go into the output: https://babeljs.io/repl#?config_lz=N4IgZglgNgpgdgQwLYxALhAJxgBygOgCsBnADxABoQdtiYAXY9AbWZHgDdLR6FMBzBkwwATGGAQBXKIwoACOAHt6ciDDkBGDfKUq1AfSSKARpo2UQRkdJjCJUOlWOT-kUrfT1MkmAF8AuhRs2AgAxvTcWJJw9BAo6CBS9IpICLGhIAFBIMS8ggC0AEyRYqGKmGnlxABqMJjEEIpwCYUADIUAzPlaFjgQODBQEHAwAAqYijiKxAhQCQAWYQDWmf6BOYqSmKEwACoAngMJVjaZQA&code_lz=DYUwLgBAHhC8EB4DOAHAhgOwHwQN4AsRhgB7AX0QHpVMsAoIA&lineWrap=true&version=7.28.4

Yes whitespace is a big issue if we start having string templates.

tsnobip avatar Sep 12 '25 12:09 tsnobip

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.

#"hello world"

fhammerschmidt avatar Sep 12 '25 13:09 fhammerschmidt