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: update eslint-plugin-ember to latest major version #2427

Merged
merged 8 commits into from
Sep 24, 2024

Conversation

shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Sep 17, 2024

📌 Summary

If merged, this PR would upgrade eslint-plugin-ember to the latest version and includes some fixes to get the lint to pass:

  • add specific overrides for gts files in showcase and components
  • add ignores to showcase, website, and components to make the linting checks pass

🔗 External links

Jira ticket: HDS-3805


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Sep 17, 2024

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

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Sep 24, 2024 3:07pm
hds-website ✅ Ready (Inspect) Visit Preview Sep 24, 2024 3:07pm

Copy link
Contributor

@aklkv aklkv left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻 One rule I would investigate ember/no-tracked-properties-from-args and maybe potentially fix

@shleewhite
Copy link
Contributor Author

Approved Percy because the changes found were just that there are new table sort by selected examples.

@didoo
Copy link
Contributor

didoo commented Sep 18, 2024

Approved Percy because the changes found were just that there are new table sort by selected examples.

@shleewhite for future reference: I think that rebasing on main would have automatically fixed the Percy tests

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.

eslint-plugin-ember v12 brings a fair amount of pivotal changes in how we should write glimmer component. while it is ok to skip some rules while we align our components with the new listing rules I want to make sure we understand these and we plan follow-up work accordingly.

one specific rule that I want us to address as part of this upgrade is empty-glimmer-component-classes – template-only Glimmer components without a backing class are much faster and lighter-weight than those with a backing classes

.changeset/loud-grapes-refuse.md Outdated Show resolved Hide resolved
packages/components/.eslintrc.cjs Outdated Show resolved Hide resolved
@@ -256,7 +256,6 @@

<Frame.Footer>
<Hds::AppFooter as |AF|>
{{! @glint-expect-error - TODO: investigate why this line fails }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alex-ju haha fixing the empty class components to templateOnly components revealed an issue with how the AppFooter component imported the types, which I fixed and then I think that fixed the weird typing issue with these files! Who woulda thought.

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.

Thanks for going the extra mile here – it paid off!

Comment on lines +10 to +13
import type { HdsAppFooterItemSignature } from './item.ts';
import type { HdsAppFooterLegalLinksSignature } from './legal-links';
import type { HdsAppFooterLinkSignature } from './link';
import type { HdsAppFooterStatusLinkSignature } from './status-link';
Copy link
Member

Choose a reason for hiding this comment

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

right!!

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.

5 participants