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

Evaluate new rules in new versions of Angular #134

Open
jattasNI opened this issue Nov 22, 2023 · 8 comments
Open

Evaluate new rules in new versions of Angular #134

jattasNI opened this issue Nov 22, 2023 · 8 comments

Comments

@jattasNI
Copy link
Collaborator

In Angular 16, angular-eslint contains some new rules and breaking changes. We will need to publish an update to our Angular styleguide that supports 16 at a minimum and preferably configures the new rules in an opinionated way.

Changelog contents copied below for easier discussion:

Features

eslint-plugin-template: All accessibility rules are now grouped together and exposed via a new @angular-eslint/template/accessibility config. See the main README.md for an example of it in action. Our schematics will explicitly add it for you in new configs, but you are free to remove it if you wish (also note the BREAKING CHANGE regarding accessibility rule names below)
builder: support reportUnusedDisableDirectives option
builder: respect using eslint.config.js (a.k.a Flat Config) if it is already present. Our schematics will only be updated to generate Flat Config once it is the default way of configuring ESLint later in the year.
bump eslint and all @typescript-eslint packages to the latest (handled for you automatically by our ng update schematic)

BREAKING CHANGES

As always, the major version primarily communicates the alignment with Angular's major version. Only Angular 16 is supported by angular-eslint 16.
Your Node version must now be 16.13.0 or higher. Node 14 support has been dropped in alignment with Angular 16, as Node 14 is EOL.
eslint-plugin: The legacy base, recommended--extra, ng-cli-compat and ng-cli-compat--formatting configs have been removed. See the updated guide on migrating from TSLint for more information: [./docs/MIGRATING_FROM_TSLINT.md](https://github.com/angular-eslint/angular-eslint/blob/main/docs/MIGRATING_FROM_TSLINT.md)
eslint-plugin-template: The legacy base config has been removed. See the updated guide on migrating from TSLint for more information: [./docs/MIGRATING_FROM_TSLINT.md](https://github.com/angular-eslint/angular-eslint/blob/main/docs/MIGRATING_FROM_TSLINT.md)
schematics: The legacy convert-tslint-to-eslint schematic has been removed. See the updated guide on migrating from TSLint for more information: [./docs/MIGRATING_FROM_TSLINT.md](https://github.com/angular-eslint/angular-eslint/blob/main/docs/MIGRATING_FROM_TSLINT.md)
eslint-plugin-template: The deprecated accessibility-label-for rule has been removed. label-has-associated-control should be used instead. An automated migration was provided when label-has-associated-control was first added, so hopefully this should not impact too many folks.
eslint-plugin-template: Previously some, but not all, accessibility related rules had a prefix of accessibility- in their name. This is inconsistent with all other rules which do not attempt to communicate why they exist in their names. Therefore that naming prefix has been removed from all rules for consistency. The accessibility related rules can easily be identified from the new @angular-eslint/template/accessibility shared config. See the main README.md for an example of it in action.
@rajsite rajsite removed the triage label Feb 22, 2024
@rajsite
Copy link
Member

rajsite commented Feb 22, 2024

Will coordinate with the design system team / if someone has time to start looking into it sooner

@jattasNI
Copy link
Collaborator Author

jattasNI commented Mar 25, 2024

#138 is adding support for Angular 16 without changing the behavior of any rules. We will use this issue to track the follow up work of enabling new rules. Here's a list from that PR:

  • I ran npm run print-available-rules and found that there were the following new, unset rules available in the updated version of @angular-eslint:
    - @angular-eslint/prefer-standalone-component
    - @angular-eslint/require-localize-metadata
    - @angular-eslint/sort-lifecycle-methods
    - @angular-eslint/template/prefer-ngsrc
    - @angular-eslint/template/prefer-self-closing-tags

Edit: all of these now have recommendations below ✅

@jattasNI jattasNI changed the title Support Angular 16 Evaluate new rules in new versions of Angular Jun 7, 2024
@jattasNI
Copy link
Collaborator Author

jattasNI commented Jun 7, 2024

In #145 we identified more new rules in 17 which are currently disabled

I ran npm run print-available-rules and found that there were the
following new, unset rules available in the updated version of
https://github.com/angular-eslint:

  • @angular-eslint/consistent-component-styles
  • @angular-eslint/no-async-lifecycle-method
  • @angular-eslint/no-duplicates-in-metadata-arrays
  • @angular-eslint/prefer-standalone
  • @angular-eslint/template/prefer-control-flow

@jattasNI
Copy link
Collaborator Author

Angular 16 rule @angular-eslint/prefer-standalone-component was deprecated and is now called prefer-standalone.
https://github.com/angular-eslint/angular-eslint/blob/main/packages/eslint-plugin/docs/rules/prefer-standalone.md

Recommendation for now is to leave this rule unset. We need to research standalone components first in the upcoming Angular feature working group. If there is an easy migration for existing non-standalone components and the working group decides we should standardize on standalone components, we would likely enable this rule. If there isn't an easy migration, we will need to decide what setting we want to encourage new projects to use.

@jattasNI
Copy link
Collaborator Author

Angular 16 rule @angular-eslint/require-localize-metadata forces you to provide a description and/or meaning for strings passed to $localize. We agreed to keep it disabled because our translators don't generally need this info and it would be a lot of churn to start providing it.

@jattasNI
Copy link
Collaborator Author

jattasNI commented Jul 25, 2024

The rule sort-lifecycle-methods enforces a file order for Angular lifecycle methods like ngOnInit and ngOnDestroy. Open questions for this rule:

  1. does it have a fixer? we think probably not
  2. how many violations in existing code? we think probably not many because we don't usually override very many lifecycle methods at once.
  3. does it enforce that they all appear together or in a certain part of the file? or just their relative order?
    • it's just relative order, they can be dispersed throughout the file. and all the methods are public so this rule won't fight with other rules enforcing file order.

3 means we are ok with the rule behavior so recommend enabling it. 1 and 2 would decide whether to fix violations in existing apps or just keep the rule disabled in those apps.

@jattasNI
Copy link
Collaborator Author

@angular-eslint/template/prefer-ngsrc encourages you to use ngsrc attribute on img attributes instead of src. This enables more optimized image loading. We don't use a lot of images but think this is probably a good rule if we ever start doing so, so recommend we enable it. It might be a good idea to sanity check it in one codebase before committing to this.

@jattasNI
Copy link
Collaborator Author

@angular-eslint/template/prefer-self-closing-tags forces templates to use the self-closing tag syntax when an element doesn't have any content. Consensus is to keep it disabled since most of us don't love self-closing tags (not valid HTML) and don't see value in encouraging their usage.

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