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

chore(components): update eslint & prettier setup #2757

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

aklkv
Copy link
Collaborator

@aklkv aklkv commented Mar 9, 2025

📌 Summary

It addresses two things

  • aligns to blueprint eslint/prettier
  • as a result of above addon is fully ready to start writing gts files whenever team is ready. This changes are coming to app it self as part of the next LTS as well but thanks to pnpm dependency isolation seems to work great we can gradually upgrade workspaces one by one

🛠️ Detailed description

📸 Screenshots

🔗 External links

Jira ticket: HDS-XXX
Figma file: [if it applies]


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Mar 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Mar 18, 2025 9:35pm
hds-website ✅ Ready (Inspect) Visit Preview Mar 18, 2025 9:35pm

Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

The proposed changes in terms of setup look good overall, aligning us with the ember blueprint.

The main reason I'm not in a position to approve yet, is the prettier bump – 3.5 is more opinionated on Sass to a point where a single change (the 'Break before breaking comma separated values', poorly implemented in my opinion) results in things like inline comments being misplaced, messing up the readability of the code. And it doesn't seem to provide a way to disable this rule via config.

I'm going to highlight a few such changes that I'm not fond of:

Screenshot 2025-03-12 at 10 35 19 Screenshot 2025-03-12 at 10 37 50 Screenshot 2025-03-12 at 10 38 48

I appreciate prettier 3.5 also comes with support for TypeScript configuration file, which is great, so I see the tradeoffs we'll have to make.

I will bring this PR to engineers attention during our sync today and aim to come up with a resolution.

alex-ju
alex-ju previously approved these changes Mar 17, 2025
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Tried my best to preserve those inline comments in CSS via prettier config to no avail, so I re-positioned them manually in 3bca98a in a way that hopefully makes sense to everyone. Comments are now either before the line or in line, never after the line.

In regards to the eslint errors, I disabled them in 9a824a3 and captured them in HDS-4656 to facilitate the upgrade preventing us from introducing new errors.

@didoo
Copy link
Contributor

didoo commented Mar 19, 2025

The proposed changes in terms of setup look good overall, aligning us with the ember blueprint.

The main reason I'm not in a position to approve yet, is the prettier bump – 3.5 is more opinionated on Sass to a point where a single change (the 'Break before breaking comma separated values', poorly implemented in my opinion) results in things like inline comments being misplaced, messing up the readability of the code. And it doesn't seem to provide a way to disable this rule via config.

Can we find a way (eg. open a PR/request/issue upstream) to avoid messing up with the Sass format, in particular the comments? For a CSS developer comments deliver a lot of meaning to the code (and their location too). The alternative for me is to have a different automation for Sass files that is less opinionated/restrictive/destructive.

@aklkv
Copy link
Collaborator Author

aklkv commented Mar 19, 2025

I can prettier ignore all scss files for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants