Skip to content

Improve jsx preserve output #7439

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.

Sign up for GitHub

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 6 commits into from
May 8, 2025
Merged

Improve jsx preserve output #7439

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
831eaa9
Indent children
nojaf May 7, 2025
6ce7b1d
Print props on different lines
nojaf May 7, 2025
5715bc2
Improve opening tag when no props are present
nojaf May 7, 2025
a3d4125
Add additional test
nojaf May 7, 2025
331765c
Merge branch 'master' into improve-jsx-output
nojaf May 8, 2025
c0fef48
Improve consistency of prop printing
nojaf May 8, 2025
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
142 changes: 105 additions & 37 deletions compiler/core/js_dump.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,34 @@ and expression_desc cxt ~(level : int) f x : cxt =
P.string f "...";
expression ~level:13 cxt f e)

and print_indented_list (f : P.t) (parent_expr_level : int) (cxt : cxt)
(items : 'a list) (print_item_func : int -> cxt -> P.t -> 'a -> cxt) : cxt =
if List.length items = 0 then cxt
else
P.group f 1 (fun () ->
(* Increment indent level by 1 for this block of items *)
P.newline f;
(* Start the block on a new, fully indented line for the first item *)
let rec process_items current_cxt_for_fold remaining_items =
match remaining_items with
| [] ->
current_cxt_for_fold
(* Base case for recursion, though initial check avoids empty items *)
| [last_item] ->
(* Print the last item, but DO NOT print a newline after it *)
print_item_func parent_expr_level current_cxt_for_fold f last_item
| current_item :: next_items ->
let cxt_after_current =
print_item_func parent_expr_level current_cxt_for_fold f
current_item
in
P.newline f;
(* Add a newline AFTER the current item, to prepare for the NEXT item *)
process_items cxt_after_current next_items
in
(* Initial call to the recursive helper; initial check ensures items is not empty *)
process_items cxt items)

and print_jsx cxt ?(spread_props : J.expression option)
?(key : J.expression option) ~(level : int) f (fnName : string)
(tag : J.expression) (fields : (string * J.expression) list) : cxt =
Expand Down Expand Up @@ -1098,19 +1126,19 @@ and print_jsx cxt ?(spread_props : J.expression option)
else None)
fields
in
let print_props cxt =
let print_props cxt props =
(* If a key is present, should be printed before the spread props,
This is to ensure tools like ESBuild use the automatic JSX runtime *)
let cxt =
match key with
| None -> cxt
| Some key ->
| Some k ->
P.string f " key={";
let cxt = expression ~level:0 cxt f key in
let cxt_k = expression ~level:0 cxt f k in
P.string f "} ";
cxt
cxt_k
in
let props = List.filter (fun (n, _) -> n <> "children") fields in

let cxt =
match spread_props with
| None -> cxt
Expand All @@ -1122,47 +1150,87 @@ and print_jsx cxt ?(spread_props : J.expression option)
in
if List.length props = 0 then cxt
else
(List.fold_left (fun acc (n, x) ->
P.space f;
let prop_name = Js_dump_property.property_key_string n in

P.string f prop_name;
P.string f "=";
P.string f "{";
let next = expression ~level:0 acc f x in
P.string f "}";
next))
cxt props
P.group f 1 (fun () ->
P.newline f;
let rec process_remaining_props acc_cxt props_to_process =
match props_to_process with
| [] -> acc_cxt
| (n, x) :: [] ->
let prop_name = Js_dump_property.property_key_string n in
P.string f prop_name;
P.string f "=";
P.string f "{";
let next_cxt = expression ~level:0 acc_cxt f x in
P.string f "}";
next_cxt
| (n, x) :: tail ->
let prop_name = Js_dump_property.property_key_string n in
P.string f prop_name;
P.string f "=";
P.string f "{";
let cxt_after_current_prop_value =
expression ~level:0 acc_cxt f x
in
P.string f "}";
P.newline f;
process_remaining_props cxt_after_current_prop_value tail
in
process_remaining_props cxt props)
in
match children_opt with
| None ->
P.string f "<";
let cxt = cxt |> print_tag |> print_props in
P.string f "/>";
cxt
| Some children ->
let child_is_jsx child =
match child.J.expression_desc with

let print_one_child expr_level_for_child current_cxt_for_child f_format
child_expr =
let child_is_jsx_itself =
match child_expr.J.expression_desc with
| J.Call (_, _, {call_transformed_jsx = is_jsx}) -> is_jsx
| _ -> false
in
if not child_is_jsx_itself then P.string f_format "{";
let next_cxt =
expression ~level:expr_level_for_child current_cxt_for_child f_format
child_expr
in
if not child_is_jsx_itself then P.string f_format "}";
next_cxt
in

P.string f "<";
let cxt = cxt |> print_tag |> print_props in
let props = List.filter (fun (n, _) -> n <> "children") fields in

(* Actual printing of JSX element starts here *)
P.string f "<";
let cxt = print_tag cxt in
let cxt = print_props cxt props in
(* print_props handles its own block and updates cxt *)

let has_multiple_props = List.length props > 0 in

match children_opt with
| None ->
(* Self-closing tag *)
if has_multiple_props then P.newline f;
P.string f "/>";
cxt
| Some children ->
(* Tag with children *)
let has_children = List.length children > 0 in
(* Newline for ">" only if props themselves were multi-line. Children alone don't push ">" to a new line. *)
if has_multiple_props then P.newline f;
P.string f ">";
if List.length children > 0 then P.newline f;

let cxt =
List.fold_left
(fun acc e ->
if not (child_is_jsx e) then P.string f "{";
let next = expression ~level acc f e in
if not (child_is_jsx e) then P.string f "}";
P.newline f;
next)
cxt children
let cxt_after_children =
if has_children then
(* Only call print_indented_list if there are children *)
print_indented_list f level cxt children print_one_child
else cxt
(* No children, no change to context here, no newlines from children block *)
in
let cxt = cxt_after_children in

(* The closing "</tag>" goes on a new line if the opening part was multi-line (due to props)
OR if there were actual children printed (which always makes the element multi-line).
*)
let element_content_was_multiline = has_multiple_props || has_children in
if element_content_was_multiline then P.newline f;

P.string f "</";
let cxt = print_tag cxt in
Expand Down
129 changes: 96 additions & 33 deletions tests/tests/src/preserve_jsx_test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,41 @@ let Icon = {
};

let _single_element_child = <div>
<h1>
{"Hello, world!"}
</h1>
<h1>
{"Hello, world!"}
</h1>
</div>;

let _multiple_element_children = <div>
<h1>
{"Hello, world!"}
</h1>
<Preserve_jsx_test$Icon/>
<h1>
{"Hello, world!"}
</h1>
<Preserve_jsx_test$Icon/>
</div>;

let _single_element_fragment = <>
{Primitive_option.some(<input/>)}
{Primitive_option.some(<input/>)}
</>;

let _multiple_element_fragment = <>
<input type={"text"}/>
<input type={"number"}/>
<input
type={"text"}
/>
<input
type={"number"}
/>
</>;

let _unary_element_with_props = <input className={"foo"} type={"text"}/>;
let _unary_element_with_props = <input
className={"foo"}
type={"text"}
/>;

let _container_element_with_props_and_children = <div className={"foo"} title={"foo"}>
{"Hello, world!"}
let _container_element_with_props_and_children = <div
className={"foo"}
title={"foo"}
>
{"Hello, world!"}
</div>;

let baseProps = {
Expand All @@ -50,43 +60,63 @@ let baseProps = {

let newrecord = {...baseProps};

let _unary_element_with_spread_props = <input {...newrecord} type={"text"}/>;
let _unary_element_with_spread_props = <input {...newrecord}
type={"text"}
/>;

let newrecord$1 = {...baseProps};

let _container_with_spread_props = <div {...newrecord$1} title={"barry"} className={"barry"}>
{"Hello, world!"}
<input type={"text"}/>
let _container_with_spread_props = <div {...newrecord$1}
title={"barry"}
className={"barry"}
>
{"Hello, world!"}
<input
Copy link
Member

@cknitt cknitt May 8, 2025

Choose a reason for hiding this comment

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

As a further improvement, maybe we could break only if there is more than one prop?

type={"text"}
/>
</div>;

let baseChildren = [
<span>
{"Hello, world!"}
{"Hello, world!"}
</span>,
<span>
{"Hello, world!"}
{"Hello, world!"}
</span>
];

let _container_with_spread_children = <div className={"barry"} title={"barry"}>
{baseChildren}
let _container_with_spread_children = <div
Copy link
Member

@cknitt cknitt May 8, 2025

Choose a reason for hiding this comment

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

I wonder if we could/should add a line break (+ indent) before the opening tag, too?

className={"barry"}
title={"barry"}
>
{baseChildren}
</div>;

let newrecord$2 = {...baseProps};

let _container_with_spread_props_and_children = <div {...newrecord$2} title={"barry"} className={"barry"}>
{baseChildren}
let _container_with_spread_props_and_children = <div {...newrecord$2}
title={"barry"}
className={"barry"}
>
{baseChildren}
</div>;

let newrecord$3 = {...baseProps};

let _unary_element_with_spread_props_keyed = <input key={"barry-key"} {...newrecord$3} type={"text"}/>;
let _unary_element_with_spread_props_keyed = <input key={"barry-key"} {...newrecord$3}
type={"text"}
/>;

let newrecord$4 = {...baseProps};

let _container_with_spread_props_keyed = <div key={"barry-key"} {...newrecord$4} title={"barry"} className={"barry"}>
{"Hello, world!"}
<input type={"text"}/>
let _container_with_spread_props_keyed = <div key={"barry-key"} {...newrecord$4}
Copy link
Member

@cknitt cknitt May 8, 2025

Choose a reason for hiding this comment

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

We should also break before the key prop and before the props spread.

title={"barry"}
className={"barry"}
>
{"Hello, world!"}
<input
type={"text"}
/>
</div>;

let _unary_element_with_only_spread_props = <input {...baseProps} />;
Expand All @@ -98,7 +128,7 @@ let A = {};

function Preserve_jsx_test$B(props) {
return <p>
{"Hello, world!"}
{"Hello, world!"}
</p>;
}

Expand All @@ -107,22 +137,54 @@ let B = {
};

let _external_component_with_children = <QueryClientProvider>
<strong/>
<Preserve_jsx_test$B/>
<strong/>
<Preserve_jsx_test$B/>
</QueryClientProvider>;

function make(props) {
return <p>
{"foo"}
{props["\\\"MyWeirdProp\""]}
{"foo"}
{props["\\\"MyWeirdProp\""]}
</p>;
}

let MyWeirdComponent = {
make: make
};

let _escaped_jsx_prop = <make MyWeirdProp={"bar"}/>;
let _escaped_jsx_prop = <make
MyWeirdProp={"bar"}
/>;

let _large_component = <div
className={"bar"}
tabIndex={1}
title={"foo"}
onClick={param => {}}
onMouseDown={param => {}}
>
<p
className={"bar"}
Copy link
Member

@cknitt cknitt May 8, 2025

Choose a reason for hiding this comment

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

Possible improvement: Do not add braces for string props.

tabIndex={1}
title={"foo"}
onClick={param => {}}
onMouseDown={param => {}}
>
{"Hello, world!"}
</p>
<strong
className={"bar"}
tabIndex={1}
title={"foo"}
onClick={param => {}}
onMouseDown={param => {}}
>
{"Hello, world!"}
</strong>
<p>
{5}
</p>
</div>;

export {
React,
Expand All @@ -148,5 +210,6 @@ export {
_external_component_with_children,
MyWeirdComponent,
_escaped_jsx_prop,
_large_component,
}
/* _single_element_child Not a pure module */
Loading