@angular-eslint/template/i18n flags attributes that don't need to be localized

[jattasNI]

In AzDO PR 256885 we saw an issue where @angular-eslint/template/i18n flagged an attribute as needing localization when it wasn't actually necessary.

In that case, the template looked like this:

<nimble-theme-provider theme="light">

The rule reported that we should add an additional il8n-theme attribute to mark the theme's value to be localized. That seems wrong, since the name of the theme is not user-visible. The Angular docs for localizing attributes explain how this works, but don't explicitly say that every attribute must be translated.

Here are the configuration options available for the rule:

interface Options {
  boundTextAllowedPattern?: string;
  /**
   * Default: `true`
   */
  checkAttributes?: boolean;
  /**
   * Default: `true`
   */
  checkId?: boolean;
  /**
   * Default: `true`
   */
  checkText?: boolean;
  /**
   * Default: `["charset","class","color","colspan","fill","formControlName","height","href","id","lang","ngClass","ngProjectAs","routerLink","src","stroke","stroke-width","style","svgIcon","tabindex","target","type","viewBox","width","xmlns"]`
   */
  ignoreAttributes?: string[];
  ignoreTags?: string[];
  requireDescription?: boolean;
}

Some possible remedies:

1. Set checkAttributes to false
2. Add theme to ignoreAttributes
3. Add nimble-theme-provider to ignoreTags
4. Add docs providing guidance for when projects should set one of the above configurations, but keep current config the same
5. Add docs about when inline suppressions are appropriate (note that inline suppressions in HTML are kind of noisy: <!-- eslint-disable-next-line @angular-eslint/template/i18n -->)
6. Other thoughts?

[mollykreis]

This issue is present for other nimble components as well. For example, "appearance" gets flagged as needing to be localized on a nimble-button.

[mure]

At first blush, checkAttributes: false seems like a reasonable default. Most of the time I would expect user visible text to be in the text content of a node, not attributes.

[jattasNI]

Another instance came up in this Soliton Argon PR. In that case the rule had flagged some mat-icons as needing i18n tags. In this case there isn't any content to internationalize, though the right approach would likely be to add accessibility tags and/or tooltips, which would require localization.

<mat-icon (click)="navigateBack()" class="back-button clickable" i18n >keyboard_arrow_left</mat-icon>
<mat-icon i18n [svgIcon]="routeData.icon" class="header-icon"></mat-icon>

[jattasNI]

Started talking about this in person today but didn't reach a decision. The ideal solution would be a way to extend the config of checkAttributes with lists mixed-in from various sources: a default list plus some nimble items plus some app-specific items, for example. We're not sure about what pattern would allow this so it would require some more research to see if it's possible.

Until we do this research our viable options include:

1. add nimble-specific items to the styleguide's default checkAttributes list. While this is ugly architecture, it will make NI Nimble clients' lives better.
2. setting checkAttributes:false. This is undesirable because it will miss attributes like title which should be localized
3. leaving the config as-is but adding docs (in Nimble, probably) that list attributes which every app should add to checkAttributes

Let's choose between these three options at our next meeting.

[jattasNI]

Begrudging consensus to start with option 1 for now. We'll hardcode the nimble attributes in a list within the styleguide and update Nimble's CONTRIBUTING docs to remind devs to keep that list up to date.

[rajsite]

When enabling we should try and include overrides for tests if possible, ie see the following:

    {
      "files": [
        "*.html"
      ],
      "extends": [
        "@ni/eslint-config-angular/template"
      ],
      "rules": {
        // Partial configuration that does not handle attributes yet
        // See https://github.com/ni/javascript-styleguide/issues/83
        "@angular-eslint/template/i18n": ["error", {
          "checkAttributes": false,
          "checkId": false
        }]
      }
    },
    {
      "files": [
        // Catch all inline templates
        // Warning: This includes inline templates in components outside of tests
        // See https://github.com/ni/javascript-styleguide/issues/83
        "*inline-template-*.component.html",
        // Do not try and lint the top-level src/index.html with the general angular component html lint rules
        "index.html"
      ],
      "rules": {
        "@angular-eslint/template/i18n": "off"
      }
    }

[rajsite]

// Catch all inline templates (which should be only in tests due to other eslint rules?)

This might not actually be true, we allow short inline templates in components outside of tests and I don't think the temporary inline template file naming scheme is flexible enough to target tests only.

Created an issue asking to be able to target inline templates of test files: https://github.com/angular-eslint/angular-eslint/issues/1023

Updated the previous snipped to clarify inline template behavior

[rajsite]

The latest version of angular eslint makes it possible to target inline templates of specific files by changing the name format of generated inline template files https://github.com/angular-eslint/angular-eslint/issues/1023#issuecomment-1166291069

[jattasNI]

This came up again in another SLE app. It revealed how many apps are currently suppressing this rule. I re-added the triage tag so we can talk about whether to bump this in priority.

[rajsite]

Prototypes worth experimenting with:

1. Export a fn to generate the rule. It's passed exported string arrays from each package to merge (nimble, systemlink shared). So packages will export and eslint rule list. Multiple packages may need to export the lists (nimble, lib angular, file lib, tag lib, asset lib) and be pulled into apps.

   Pros: Packages responsible for keeping lint in sync.

   Implementation thoughts:

   • Don't mark i18n in tests (see above linked comment about opting out in tests) but do require in example apps.
     • Reason is to help capture changes to the exported i18n lists.
   • Disable the i18n attribute in the default exported rules. Use the fn to generate the rule with attr lists
   • The fn should allow you to pass a custom list for your app (likely just takes all args as arrays to merge)
   • Define the convention for packages to export the attribute list
2. Thrown out: Only managing the hard coded list in the eslint style guide. Issues: to do it "right" would want peer dependencies on the supported pkg. cons: easy to get out of sync with elements in packages, bubble up of changes between lint and packages.
3. Disable i18n checking for attributes.

[jattasNI]

I prototyped a possible fix to this on an innovation day. In my initial attempt I kept all the lists of attributes to ignore in the client shared library. We discussed this approach and decided that we'd prefer to keep the lists in the styleguide so that the rule can be configured by default to use them. We'd also export the lists so that apps could still include them if they wanted to override the rule configuration.

This means the styleguide will have a global list of attributes for all NI including things from Nimble, SystemLinkShared, and any commonly used 3rd party packages like Material. While that's architecturally impure, it seems simpler than having each package export its own list of attributes and less annoying to configure than having each app maintain a list.

[rbell517]

I'd like to bump the priority of this. We have now gotten a bug because an attribute was localized that shouldn't have been (in this case slTableColumnId), breaking behavior in that locale. This is not just an annoyance, its an active impediment to making a quality product.

I agree with the suggestion to single source this in the styleguide, regardless of the attribute's source package. I hope to have time today to take a stab at this using @jattasNI 's list as a starting point.