ni / javascript-styleguide

JavaScript and TypeScript Style Guide
MIT License
9 stars 9 forks source link

Evaluate new rules in new versions of Angular #134

Open jattasNI opened 9 months ago

jattasNI commented 9 months ago

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 commented 6 months ago

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

jattasNI commented 5 months ago

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 commented 3 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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.