100phlecs / tailwind_formatter

Sorts tailwind classes within elixir projects
https://hexdocs.pm/tailwind_formatter
MIT License
111 stars 11 forks source link

Formatter inserts incorrect `}` #16

Closed krns closed 1 year ago

krns commented 1 year ago

The first time I tried the formatter in one of our projects it worked. The second time I got an FunctionClauseError no function clause matching in Phoenix.LiveView.HTMLFormatter.to_tree/4.

It turns out that the first run of the tailwind formatter formats the following

    <div class={"h-6 " <> if @active, do: "bg-white", else: "bg-red"}></div>

to

    <div class={"h-6"} <> if @active, do: "bg-white", else: "bg-red"}></div>

And this is invalid code because of the inserted } :-)

krns commented 1 year ago

I tried to tell the formatter not to format this code with phx-no-format. Seems like the tailwind formatter isn't respecting the phx-no-format attribute.

I really like the idea of a tailwind formatter and think supporting the phx-no-format attribute would be a nice workaround for various edge cases in the future.

100phlecs commented 1 year ago

Thanks for the report! I've not come across that style for class interpolation before. It's an interesting case, would need to rethink the regex strategy to predominantly target { & } as terminators instead of {" & "} or just plain quotes (as I've been expecting only a string as the inner block)

I do like the idea of supporting the phx-no-format attribute. The reason that works seamlessly for the Liveview.HTMLFormatter is because they're making an AST of all the HTML code & attributes, so they can then query if that attribute exists on the node before formatting.

In this solution, we're using regex, so I can't think of an immediately obvious way to correlate the primary regex (basically searching for class={"*"} right now) and associating it with the HTML node it's in.

A couple of options:

  1. A quick compromise would be to have some sort of tw-no-format as a class name, which would be easy to check because it's within the class block.
  2. Support the phx-no-format as well, but needs some way to associate the class list with the html block it is in. Maybe make an AST as well? Something to think about.

In either case, thanks for the test case. I'll check for <> as another "elixir function" to extract. Let me know if you run into any other sorting issues :^)

100phlecs commented 1 year ago

I've set up some extremely naive support for string concatenation in #17

Todo:

I'm curious what other type of elixir blocks that aren't accounted for, beyond this concatenation.

krns commented 1 year ago

Thank you very much for your thoughts. Just tested your solution successfully.

This kind of conditional class construction is a legacy from some of our older projects, before LiveView features like assign_new and declarative assigns were available.

100phlecs commented 1 year ago

Fixed with #30