Unnecessary format changes in JSX between 10.0.1 and 10.1.0-rc.1

[cknitt]

I just installed 10.1.0-rc.1 in a project of ours for testing and ran the formatter across the code. This resulted in 83 files changed.

The reason was that as a side effect of #664, JSX tags are now formatted differently, as can also be seen in the test result diffs in this PR.

Before:

<svg
  viewBox="0 0 24 24"
  width="24"
  height="24"
  stroke="currentColor"
  strokeWidth="2"
  fill="none"
  strokeLinecap="round"
  strokeLinejoin="round"
  className="text-gray-400">
  <polyline points="6 9 12 15 18 9" />
</svg>

After:

<svg
  viewBox="0 0 24 24"
  width="24"
  height="24"
  stroke="currentColor"
  strokeWidth="2"
  fill="none"
  strokeLinecap="round"
  strokeLinejoin="round"
  className="text-gray-400"
>
  <polyline points="6 9 12 15 18 9" />
</svg>

While it may be debatable which style is better, I think the difference is not worth causing so many format changes in users' codebases.

[ah-yu]

Hello, I investigated some popular formatter(prettier, deno format), and they put the > on a new line. Probably this code style is more familiar to Js users.

It seems like a breaking change maybe it is better to release in the next major if you are in favor of this change.

[IwanKaramazow]

I investigated some popular formatter(prettier, deno format), and they put the > on a new line. Probably this code style is more familiar to Js users.

Don't forget our existing users. We should try to reserve this for v11 and restore the previous way of formatting.

[cristianoc]

We need to revert this change on branch 10.1 and add it to master with a changelog listing as breaking change.

[cristianoc]

Since this is the syntax repo, seems best to revert here and leave an open PR to add it back.
And an issue on v11 milestone to add it later.

[cknitt]

Actually for Prettier this is a configuration option (bracketSameLine). It's true that bracketSameLine = false is Prettier's default though (whereas what we have in ReScript 10.0 corresponds to bracketSameLine = true).

However, we are not following Prettier's defaults in other places either, e.g. Prettier 2.0+ has arrowParens = "always" as a default whereas ReScript's formatting corresponds to arrowParens = "avoid".

I agree that, if we do this format change, we should do it in v11, but I am not really convinced that we should do it at all.

#664 was about fixing a missing comments case - can this be solved without introducing the format change?

[IwanKaramazow]

#664 was about fixing a missing comments case - can this be solved without introducing the format change?

Just took a quick look, it seems possible although it requires a bit more logic in the case of

<A
   b=1
   // trailing single line comment
>
  children
</A>

[cknitt]

@ah-yu @cristianoc I tested again with latest syntax and am now seeing changes like this one in our codebase:

    <ChatComponents.Sidebar
      variant={own ? #Own : #Other}
      restricted
      spaceBetweenButtons={message.location->Option.isSome}>
      {children}
    </ChatComponents.Sidebar>

=>

    <ChatComponents.Sidebar
      variant={own ? #Own : #Other} restricted spaceBetweenButtons={message.location->Option.isSome}>
      {children}
    </ChatComponents.Sidebar>

[ah-yu]

Looks fine on my computer

zheyzhang@HANM18365 syntax % dune exec -- rescript -print res -width 80 test.res
<ChatComponents.Sidebar                
  variant={own ? #Own : #Other}
  restricted
  spaceBetweenButtons={message.location->Option.isSome}>
  {children}
</ChatComponents.Sidebar>
zheyzhang@HANM18365 syntax % 

[ah-yu]

I confirm that. But it seems that is not introduced in my PR. I revert my changes and that issue still exists.

See #684 (comment)

@cknitt

[ah-yu]

And it's weird. I use dune exec -- rescript -print res -width 80 test.res to test, the result is right, but with make test, the result is wrong

[ah-yu]

<ChatComponents.Sidebar
  variant={own ? #Own : #Other} restricted spaceBetweenButton={message.location->Option.isSome}>
  {children}
</ChatComponents.Sidebar>

This seems to be the correct behavior if the default width is 100.

I tested with rescript@9.1.4 and rescript@10.0.1 and the output is the same as above