Skip to content
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

A new direction perhaps? #66

Open
adamroyle opened this issue Jan 20, 2025 · 2 comments
Open

A new direction perhaps? #66

adamroyle opened this issue Jan 20, 2025 · 2 comments

Comments

@adamroyle
Copy link

Hi @100phlecs !

I have created a Tailwind formatter project forked from your project. At first, I was wanting to add some features, but then I realised I needed to take a difference approach to get the outcome I wanted.

Here is my current version I have been using locally as I've been developing my app (I haven't published this to hex). The code is a little messy but it has been working well for my use-case.

https://github.com/adamroyle/tailwind_formatter/tree/feat/refactor

Features

  • Accurate sorting for all tailwind classes, including custom themes and variants (eg. text-mycolor or pt-[55px] or data-[active]:bg-white)
  • Trims whitespace from start and end of class strings
  • On-demand class ordering (no need to pre-generate variants)
  • ETS caching for speed improvements

How it works

When a request for formatting comes in (either through the LSP or mix format) Tailwind runs on just that one file. The resulting css is parsed to determine the class order. The class order is persisted to ETS so subsequent format requests for the same file won't execute Tailwind unless new classes are present.

Caveats

  • LSP format performance - Initial format is not instant, but usually less than 1 second. Subsequent formats are cached and will only cause a delay if new classes are detected and Tailwind needs to run again
  • mix format performance - This runs Tailwind for every file so there is a delay for each file, but since mix format runs in parallel I haven't found it to be a major issue
  • Tailwind 4 not yet supported - At the moment I am using a custom input css file (priv/app.css) but v4 could be supported in future
  • Multiple profiles not yet supported - I tried to get this working by detecting the profile automatically but ran into some blockers. Would love to solve but don't have a requirement for this feature currently.

Why am I writing all this?

I think the benefits easily outweigh the negatives and would prefer to contribute to this project rather than create and maintain my own hex package. What do you think?

@100phlecs
Copy link
Owner

100phlecs commented Jan 27, 2025

Hi, thanks for reaching out! There are a few concerns, but overall I think this new direction could be interesting, as the current DX of loading the classes/variants isn't the greatest. And will cause issues with the new v4 .css configuration. Which, from what I understand, this direction would eliminate that "extraction" step.

Trims whitespace from start and end of class strings

One problem here is we support the case where someone concatenates the string, e.g.:

<div class={"text-red-600 " <> if @active, do: "bg-blue-300"} />

But I think we could figure out whether to leave the space dangling or not, as trimming would be nice to have.

Some additional thoughts:

  • Would it be possible/easier to reference the output of a single .css tailwind file instead of running tailwind each time? Basically, call the tailwind executable on the entire project, save the ordering in ETS and then go through the formatting
  • It sounds like we're taking advantage of the "sort order" of the .css file, which I think tailwind does before outputting to css. But if this is not the case, that may cause some issues
  • We may lose the strict variant ordering with these changes. I wonder if there is a way to preserve that -- but it may be too much of a headache

If we can address the concatenation issue and the performance difference isn't that much noticeable, then this direction would be welcomed, as it solves the "v4" issue as well as "config extraction" grievances. I also notice some tests commented out in the diff which ideally we can get passing again, depending on the new constraints 👍

edit:

I am also waiting on how the new v4 tailwind will be integrated with phoenix -- especially the heroicons plugin. phoenixframework/tailwind#107

What do you think about this new approach, @aptinio? It doesn't seem that too far off from #47, just eliminating the extraction part by running the executable instead

@adamroyle
Copy link
Author

@100phlecs

Instead of using concatenation, I prefer to use the list shorthand which doesn't require leading or trailing spaces.

<div class={["text-red-600", @active && "bg-blue-300"]} />

It sounds like we're taking advantage of the "sort order" of the .css file, which I think tailwind does before outputting to css. But if this is not the case, that may cause some issues

Yes, Tailwind does sort the classes correctly before outputting them. The few exceptions (.group and .peer) are handled within the TailwindFormatter.ClassMap.get_order/2 function.

We may lose the strict variant ordering with these changes. I wonder if there is a way to preserve that -- but it may be too much of a headache

I noticed that the prettier plugin in JS doesn't support variant ordering so I removed this functionality.

Would it be possible/easier to reference the output of a single .css tailwind file instead of running tailwind each time? Basically, call the tailwind executable on the entire project, save the ordering in ETS and then go through the formatting

That was my first approach but it didn't work out. I couldn't find a way to distinguish between the LSP formatting a single file vs mix format formatting the whole project.

This approach doesn't work with the LSP since Tailwind wouldn't see any new classes since they are not persisted to the filesystem yet (when using format on save).

My opinion is that LSP performance is the priority for DX. In my experience the slowdown when using the LSP is generally less than 500ms. It's also important to note this delay only happens when new classes are detected. Most of the time you are re-using the same classes over again so formatting is instant.

After I simplified my approach and ran Tailwind for each file, I realised mix format formats 10 files concurrently (on my M1 MacBook with 10 cores). I only have about 20 files that contain HEEx so the real-world time slowdown wasn't a big deal.

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

No branches or pull requests

2 participants