Put each jsx tag on own line #574

nkrkv · 2022-06-22T14:34:13Z

Closes #570

This is my first lines of code in OCaml, so I’m not sure I’m doing it right 😺

cknitt · 2022-06-23T05:07:49Z

This is awesome! Thanks a lot!

Just wondering about performance when computing hasNestedJsx - I think we could avoid the list creation done by ParsetreeViewer.collectListExpressions here.

/cc @IwanKaramazow

cristianoc · 2022-06-23T05:12:38Z

This is awesome! Thanks a lot!

Just wondering about performance when computing hasNestedJsx - I think we could avoid the list creation done by ParsetreeViewer.collectListExpressions here.

/cc @IwanKaramazow

Indeed.
Bonus point: do it with without allocating.
For example, creating a list and walking a list is allocating.
Technically, even creating a single closure to pass to List.exists would be allocating, but that's a very small allocation.

nkrkv · 2022-06-23T13:26:16Z

I think we could avoid the list creation done by ParsetreeViewer.collectListExpressions here.
Bonus point: do it with without allocating.

Thank you gentlemen. Would you point me a little bit, how can I traverse expression children without creating a list? Just a function name, a very rough idea, or a hint is enough. Otherwise I can go wrong direction again :)

cknitt · 2022-06-23T13:37:46Z

I'd say iterate/recurse like ParsetreeViewer.collectListExpressions does, but do not build a list, instead directly check each item if its a JSX expression (ParsetreeViewer.isJsxExpression). Return true if a JSX expression was found, otherwise continue recursion.

cristianoc · 2022-06-23T13:58:28Z

I'd say iterate/recurse like ParsetreeViewer.collectListExpressions does, but do not build a list, instead directly check each item if its a JSX expression (ParsetreeViewer.isJsxExpression). Return true if a JSX expression was found, otherwise continue recursion.

So create a little helper function which is a recursive traversal.
Then call that function.
This will also take care of the little code duplication in the current PR.

nkrkv · 2022-06-23T19:55:27Z

Thanks for pointing the direction. I think I’ve done it, not sure though :) Would you look at the newest commit?

cristianoc

Looks great.
Some tiny style comments.

cristianoc · 2022-06-23T20:41:27Z

src/res_printer.ml

+  | Pexp_construct ({txt = Longident.Lident "[]"}, _) -> false
+  | Pexp_construct
+      ( {txt = Longident.Lident "::"},
+        Some {pexp_desc = Pexp_tuple (hd :: [tail])} ) ->


nit: this is normally written [hd, tail].

Hmmm, this gives me:

File "src/res_printer.ml", line 106, characters 39-44: 106 | Some {pexp_desc = Pexp_tuple ([hd, _])} ) -> ^^^^^ Error: This pattern matches values of type 'a * 'b but a pattern was expected which matches values of type Parsetree.expression

My OCaml knowledge is not enough to understand why is it different. Perhaps, type inference fails somehow in the [hd, tail] case. Anyway, I’ve stolen this pattern from ParsetreeViewer.collectListExpressions.

Sorry [hd; tail] not [hd, tail].

As a final step, before and after: make bench ... If you are on macos, that is

I’m on Linux and have no access to a macOS machine, unfortunately. Would someone help with running bench?

Will do once the PR is final.

Sorry [hd; tail] not [hd, tail]

Muscle memory :) Fixed it.

cristianoc · 2022-06-23T20:42:15Z

src/res_printer.ml

+  | Pexp_construct
+      ( {txt = Longident.Lident "::"},
+        Some {pexp_desc = Pexp_tuple (hd :: [tail])} ) ->
+    hasNestedJsx hd || hasNestedJsx tail


Should this be ParsetreeViewer.isJsxExpression hd ? and the next line do nothing? (and just return false)

Yes. Otherwise it behaves differently than before this commit (see the spread change in this commit which I think we do not want).

Ah, now I understand what was the reason for spread behavior change. Fixed it, and the function has became even more simple and even non-rec. Thanks.

cristianoc · 2022-06-23T20:45:52Z

This is looking great.
As a final step, before and after: make bench. See what it says about allocation.

cristianoc · 2022-06-23T20:47:00Z

If you are on macos, that is. Does not work on other architectures at the moment.

nkrkv · 2022-06-24T08:42:32Z

As a final step, before and after: make bench ... If you are on macos, that is

I’m on Linux and have no access to a macOS machine, unfortunately. Would someone help with running bench?

src/res_printer.ml

cknitt · 2022-06-24T09:28:06Z

Hmm, and what do we actually do if we have a lot of children, but none is a JSX expression?

Like

<A> {a} {b} {c} {d} {e} </A>

Maybe the logic would need to be something like: expand if a child is a JSX expression or if there is more than 1 child?

What do you think @cristianoc?

cristianoc · 2022-06-24T09:48:46Z

Hmm, and what do we actually do if we have a lot of children, but none is a JSX expression?

Like
<A> {a} {b} {c} {d} {e} </A>
Maybe the logic would need to be something like: expand if a child is a JSX expression or if there is more than 1 child?

What do you think @cristianoc?

What does prettier do?

nkrkv · 2022-06-24T09:58:35Z

What does prettier do?

<A>{a} {b} {c} {d}</A>;

👇

<A>
  {a} {b} {c} {d}
</A>;

Do we want the same? It would made the algorithm slightly more complex (line separator between children and after/before tags differs) but doable nevertheless.

cknitt · 2022-06-24T10:02:42Z

I tried

<A>{a}{b}{c}{d}</A>;

in one of our JS projects using prettier and got

<A>
  {a}
  {b}
  {c}
  {d}
</A>;

Maybe our prettier config is different than yours.

cknitt · 2022-06-24T10:04:02Z

And

<A>{a}</A>;

stays unmodified for me.

nkrkv · 2022-06-24T10:09:07Z

It is from their playground with default settings

Replicating the behavior with your prettier config should be easy to implement

cknitt · 2022-06-24T10:14:23Z

Ah, I know, it's because of the spaces, see https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeAggPmAQwL7ABGBYBAJgXHqgPToAEmA3ADpRsbb71F4+k8KPKrQbM2IADQgIABxgBLaAGdkoHACcNEAO4AFTQlUocAGx04AnqumENOMAGs4MAMo4AtnAAyCqHGQAMzNlOFt7JxdXWQc-AHNkGA0AVzCQOA9CODIybO8cKDjknDi4ADEIDQ8cGEVC5BAcZJgIKRAACxgPUwB1doV4ZRiwOFcjAYUANwHLBrBlGxA-UI0YPXs46qCQtIArZQAPV3jTOABFZIh4bdNQ6RiNFYaYS1k4ZTANBXk22W1Qnr2WQNMwWaxtACOl3g6zkxkaygAtP5stk2ho4FCFBj1iUtkhgrc0qEPApEiliSdztCAgSdtIYDhCD0FGQYO1kAAmBn2BSmeIAYQgHnx6WUAFY2slQgAVJnGQl3ECTVIASSguVgrk+3xg6A1rhepxuSr+fhgLLZHKQAEZOQAGPB4IA

These are significant in JS TSX, but not in ReScript, in ReScript we would need to explicitly have {React.string(" ")}.

So I think we should look at the behavior without spaces.

cknitt · 2022-06-24T10:34:05Z

Maybe the logic would need to be something like: expand if a child is a JSX expression or if there is more than 1 child?

actually matches Prettier's behavior as far as I can see:

https://prettier.io/playground/#N4Igxg9gdgLgprEAuEAeAggPlQIQPTZ7oAEmA3ADpRUbb7YDCBqRpl1UtuBwYAvixLkqNLMABGApoSHtRmYAEM+Ega2EdaqwWyogANCAgAHGAEtoAZ2ShFAJzsQA7gAV7CaykUAbJ4oCe1obidopgANZwMADKigC2cAAyZlBwyABmPpZwwaERUdHGYSkA5sgwdgCuOSBwceJwACaNTYmKUCWViiVwAGIQdnGKMOYdyCCKlTAQBiAAFjBx3gDqc2bwlkVgcNEe62YAbuv+42CWQSAp2XYwLqElQxlZNQBWlgAe0aXecACKlRB4E9vNlDEU7NdxuJFA1vLNjHYUjBlmZGjA5sgABwABjBjmyy1CxnGCLg1wOaUMAEcAfA7iZPBNLABaVJNJqzOxwGlmLl3bqPJCZEE1bJxMzlKqi75-WlpIXPQwwGEotEYpAAJiVoTM3lKDAgcUFtUsAFZZpVsgAVGGeYWgkAHaoASSgLVg0TAiNM6Dd0Rg-h+wOyfD4QA

nkrkv · 2022-06-24T12:25:35Z

Done. Now the behavior aligns with Prettier

cknitt · 2022-06-24T13:36:29Z

tests/printer/other/expected/fatSlider.res.txt

    <div className="meter text-4xl font-bold text-gray-600 mt-4">
-      {values[0]->Js.Int.toString->string} {meterSuffix->Belt.Option.getWithDefault(null)}
+      {values[0]->Js.Int.toString->string}
+      {meterSuffix->Belt.Option.getWithDefault(null)}


Definitely an improvement in this example!

cknitt · 2022-06-24T13:37:58Z

I think it can be done in one match without the "isConent":

let hasNestedJsxOrMoreThanOneChild expr =
  let rec loop inRecursion expr =
    match expr.Parsetree.pexp_desc with
    | Pexp_construct
        ({txt = Longident.Lident "::"}, Some {pexp_desc = Pexp_tuple [hd; tail]})
      ->
      if inRecursion || ParsetreeViewer.isJsxExpression hd then true
      else loop true tail
    | _ -> false
  in
  loop false expr

nkrkv · 2022-06-24T14:58:06Z

Yes, thanks. This is definitely more clear and I like the name improvement.

cknitt

Thanks! 👍

nkrkv added 3 commits June 22, 2022 17:19

Put each JSX tag on its own line

7d8d05d

Put each JSX tag nested in fragment on its own line

155cdaa

Refactor code to be more DRY

ddd58b1

Avoid list allocations while looking for nested JSX exprs

af9fa3b

nkrkv force-pushed the put-each-jsx-tag-on-own-line branch from 56cbf9d to af9fa3b Compare June 23, 2022 19:57

cristianoc approved these changes Jun 23, 2022

View reviewed changes

Simplify “has nested jsx” check, do not recurse in non-list child exprs

6635929

cristianoc reviewed Jun 24, 2022

View reviewed changes

src/res_printer.ml Outdated Show resolved Hide resolved

nkrkv added 2 commits June 24, 2022 12:49

Bring back recursion while looking for nested JSX, add more tests

9c68ded

Minor code styling

97cb87c

Put JSX children on own lines when more than two found

50ebfa9

cknitt reviewed Jun 24, 2022

View reviewed changes

Simplify and more precise name for nested jsx search function

47c697d

cknitt approved these changes Jun 24, 2022

View reviewed changes

cristianoc merged commit 2074447 into rescript-lang:master Jun 24, 2022

nkrkv deleted the put-each-jsx-tag-on-own-line branch June 24, 2022 16:12

Put each jsx tag on own line #574

Put each jsx tag on own line #574

Uh oh!

Conversation

nkrkv commented Jun 22, 2022

Uh oh!

cknitt commented Jun 23, 2022

Uh oh!

cristianoc commented Jun 23, 2022

Uh oh!

nkrkv commented Jun 23, 2022

Uh oh!

cknitt commented Jun 23, 2022

Uh oh!

cristianoc commented Jun 23, 2022

Uh oh!

nkrkv commented Jun 23, 2022

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

cristianoc Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

nkrkv Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cristianoc Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

nkrkv Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

nkrkv Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc commented Jun 23, 2022

Uh oh!

cristianoc commented Jun 23, 2022

Uh oh!

nkrkv commented Jun 24, 2022

Uh oh!

Uh oh!

cknitt commented Jun 24, 2022

Uh oh!

cristianoc commented Jun 24, 2022

Uh oh!

nkrkv commented Jun 24, 2022

Uh oh!

cknitt commented Jun 24, 2022

Uh oh!

cknitt commented Jun 24, 2022

Uh oh!

nkrkv commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cknitt commented Jun 24, 2022

Uh oh!

cknitt commented Jun 24, 2022

Uh oh!

nkrkv commented Jun 24, 2022

Uh oh!

cknitt Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkrkv commented Jun 24, 2022

nkrkv Jun 24, 2022 •

edited

Loading

cristianoc Jun 24, 2022 •

edited

Loading

nkrkv commented Jun 24, 2022 •

edited

Loading

cknitt commented Jun 24, 2022 •

edited

Loading