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.

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

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented May 7, 2025

Fixes #7434

Copy link

pkg-pr-new bot commented May 7, 2025

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript-lang/rescript@7439

@rescript/darwin-x64

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

@rescript/linux-arm64

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

@rescript/linux-x64

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

@rescript/win32-x64

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

@rescript/darwin-arm64

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

commit: c0fef48

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This is great.

Some remarks:

</span>
];

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

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"}
>
{"Hello, world!"}
<input
Copy link
Member

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?

onMouseDown={param => {}}
>
<p
className={"bar"}
Copy link
Member

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.

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

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.

@zth
Copy link
Collaborator

zth commented May 8, 2025

@cknitt just my 5c, but it honestly feels like wasted effort to go into this much details in formatting. IMO the current state in this PR is good (great!) enough, and more details/improvements should be solved in a better way than us going down the rabbit hole of trying to replicate Prettier et al.

A better long term solution that we've talked a bit about is that Rewatch could potentially do real formatting via Biome or similar, on all of our generated JS. We had a discussion about that with @jfrolich on the retreat. Then we'll have solved all of our formatting issues for the foreseeable future.

@cknitt
Copy link
Member

cknitt commented May 8, 2025

I am a bit skeptical of the Biome idea. But I agree that replicating prettier is a non-goal.

My comments are mostly just suggestions, except for the last one where the output is inconsistent with the other examples.

I would say let's do what can be done with little effort.

@nojaf
Copy link
Collaborator Author

nojaf commented May 8, 2025

I would like to merge this as is. I consider this "improved." Others can still send PRs to their heart's content.

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks a lot! Great work!

@nojaf nojaf merged commit a745e6f into rescript-lang:master May 8, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSX preserve mode: better output formatting (indentation)
3 participants