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

feat(fmt): support CSS, SCSS, Sass and Less #24870

Merged
merged 3 commits into from
Aug 9, 2024
Merged

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Aug 4, 2024

This PR integrates Malva into deno fmt, which introduces the ability to format CSS, SCSS, Sass and Less files.

On Linux x64 6.10, this PR increases about 800KiB:

󰈺  deno  fmt-css  1                                                                                                                                                                                             
❯ wc -c target/release/deno
125168728 target/release/deno

󰈺  deno  main  1                                                                                                                                                                                                
❯ wc -c target/release/deno
124349456 target/release/deno

@castarco
Copy link

castarco commented Aug 4, 2024

@g-plane any chance of this being integrated into Biomejs as well? 🥺

@g-plane
Copy link
Contributor Author

g-plane commented Aug 4, 2024

No. Underlying AST structures can't be shared. If you want to format CSS, SCSS, Sass and Less with Malva but more configurable, you can also use Malva as a dprint plugin, while using Biome to format TypeScript or JavaScript.

@bartlomieju bartlomieju added this to the 1.46 milestone Aug 5, 2024
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

Exciting! Looks go to me. We'll merge this soon.

@crowlKats
Copy link
Member

@g-plane
Copy link
Contributor Author

g-plane commented Aug 6, 2024

@crowlKats Fixed. Please try again.

@crowlKats
Copy link
Member

@g-plane tried on the above files, and works nicely. tried with JSR though, and am getting

Error formatting: /Users/crowlkats/projects/deno/denoland/jsr/frontend/static/styles.css
   syntax error: expect token `{`, but found `(`

in this file https://github.com/jsr-io/jsr/blob/main/frontend/static/styles.css.
The error message itself is not very useful.

And one more tweak:
Screenshot 2024-08-06 at 17 56 19
these are now a bit more annoying harder to read, it would be nice if they would be indented, like this:
Screenshot 2024-08-06 at 17 56 58

@g-plane
Copy link
Contributor Author

g-plane commented Aug 6, 2024

Here's an invalid syntax:
https://github.com/jsr-io/jsr/blob/7339d0d05b1536fed94a3ba45ecb106e58dd3e17/frontend/static/styles.css#L147

LightningCSS also doesn't allow this: playground, and Biome also doesn't allow this: playground, so you may need to update it.

@dsherret
Copy link
Member

dsherret commented Aug 7, 2024

@g-plane do you know why it doesn't say the line and column number of the syntax error? It would be nice to improve that.

@g-plane
Copy link
Contributor Author

g-plane commented Aug 7, 2024

The parser doesn't provide line and column information, but it provides offset position so we can do some calculations.

@crowlKats
Copy link
Member

Here's an invalid syntax:
https://github.com/jsr-io/jsr/blob/7339d0d05b1536fed94a3ba45ecb106e58dd3e17/frontend/static/styles.css#L147

I see, however since tailwind is extremely spread throughout the deno styling ecosystem, especially with fresh supporting it out of the box, so we should support this, even if its not spec compliant

@g-plane
Copy link
Contributor Author

g-plane commented Aug 7, 2024

@dsherret I found that Deno is already using codespan-reporting crate, and I think we can re-use this to generate better error message.

@g-plane
Copy link
Contributor Author

g-plane commented Aug 7, 2024

@crowlKats Please test more repositories as possible so that I can fix them together, instead of releasing new version of formatter again and again.

@g-plane
Copy link
Contributor Author

g-plane commented Aug 8, 2024

@crowlKats @dsherret All problems are solved.

@bartlomieju
Copy link
Member

bartlomieju commented Aug 8, 2024

Thanks @g-plane! We'll check it out and merge. Since it's gonna be an unstable feature for the time being it's fine to have more bugs, we'll make sure to open issues if we get any reports.

Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

All our cases seem covered & working, so LGTM!

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM!

@bartlomieju bartlomieju merged commit 8288434 into denoland:main Aug 9, 2024
17 checks passed
@marvinhagemeister
Copy link
Contributor

@g-plane This is amazing! Super excited about having this in Deno 💯

@g-plane g-plane deleted the fmt-css branch August 13, 2024 07:29
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.

6 participants