Update @angular-eslint/template/i18n to ignore common nimble and sl-lib attributes #140

rbell517 · 2024-05-03T17:42:30Z

Justification

Update @angular-eslint/template/i18n to ignore common nimble and sl-lib attributes (and excludes tests of the name *.spec.ts with inline templates).

fixes #83

Implementation

A new file template/options.js is added to list all well-known attributes that should not be localized for using in NI javascript codebases. This includes common nimble attributes, systemlink-lib-angular attributes, and attributes from commonly used third-party libraries.

This list is used to modify the @angular-eslint/template/i18n eslint rule to ignore any attribute in the list by default. If apps need to change this to ignore only nimble attributes, they have the option to do that by overriding the rule's config in their app's eslintrc.

Testing

I have made modifications to the angular test project to include attributes on the two sample templates that should either be ignored or not ignored and verified the eslint rule modification is being correctly applied in each.
Added a test to verify custom attributes can be added to ignore lists by apps.

jattasNI

Looks great, thanks for tackling this! I'm checking with other owners to see if they want to review; I'll approve once I hear back.

change/@ni-eslint-config-angular-ff70c9b2-14ea-4eb3-9c0e-0de3cdb388ec.json

…ib attributes Change files

packages/eslint-config-angular/i18n-attributes-ignore.js

packages/eslint-config-angular/template.js

jattasNI

You can complete this once @rajsite approves, no need to wait on the remaining reviewers.

packages/eslint-config-angular/template.js

tests/angular/custom-ignore-attributes/.eslintrc.js

rajsite · 2024-05-10T22:27:39Z

packages/eslint-config-angular/options.js

+// CSS selector style element[attribute] ignore entries
+// which can be used to scope attribute ignores to specific
+// elements.
+const ignoreAttributes = {


@jattasNI we have this rule disable in nimble: https://github.com/ni/nimble/blob/4e595cc0e67a9903510ba60da17afdb1bf2643d6/packages/angular-workspace/.eslintrc.js#L109

Maybe we should locally enable it with a local build of the changes to see if the example app reveals anymore attributes that need to be marked? How confident are we in the completeness of the current list / should we do any work preemptively to shore-it up for nimble?

After this change goes in maybe we should enable i18n on the example app as a way to catch attributes to add to this list sooner?

I am not very confident in the completeness of the list. In my innovation day prototype PR I generated it by trial and error on the SystemLinkShared libraries so it could easily be missing APIs that are used by apps (or by nobody yet) if they're not used by a shared library. I think this PR largely uses that list, so it's probably incomplete.

I am fine with making the list comprehensive either before or after completing this PR. I suspect that adopting the package in SLE apps will reveal more APIs than adopting it in the Nimble example app since the latter doesn't exercise APIs very comprehensively. But creating a local package and locally adopting it in every app is a lot of busy work so if @rbell517 would rather wait for Renovate to do that work on a production build and then add any missing attributes as a follow up PR, that's ok by me.

Similar thought on enabling i18n on the example app. I don't object to giving it a shot but I think it'll miss a lot of new APIs.

But creating a local package and locally adopting it in every app is a lot of busy work so if @rbell517 would rather wait for Renovate to do that work on a production build and then add any missing attributes as a follow up PR, that's ok by me.

If @rbell517 is okay keeping an eye out on rennovate PRs and helping message / coral folks running into issues post merge that works for me 👍

I suspect that adopting the package in SLE apps will reveal more APIs than adopting it in the Nimble example app since the latter doesn't exercise APIs very comprehensively.

Yea that's true. The test suite is intended to test the angular bindings exhaustively though. Maybe we should turn on i18n in nimble-angular's test suite in the future 🤔

@rbell517 if you are okay with this direction then we are ready to ship (resolve the comment and should automerge)

Offline sounds like @rbell517 has additional feedback to give so cancelling autocomplete to prevent accidental release.

Just so there are not any misunderstandings on the impact of this change, uptaking this new version in libraries and apps will not by itself "reveal more APIs." This change means errors will no longer be generated for the listed attributes when it doesn't have an i18n attribute, but it will not generate errors when the listed attributes do have an i18n attribute. That is just a limitation of the eslint rule. It also does nothing to the standard [attribute]="'value'" trick to avoid the eslint rule without suppressing. The net is this is strictly a loosening of the rule, no new errors should be generated by uptaking the new version, so there shouldn't be any need to monitor Renovate builds.

What I've done instead is use some regex look for cases we worked around the i18n rule (\[[a-zA-Z-]+\]="'.*'") or have added a bad i18n- attribute (i18n-[a-zA-Z-]+) and scrubbed the results for SystemLinkShared, TestInsights, LabManagement, and SystemsManagement. This revealed 5 instances of bad i18n attributes (all Lab Management), which is actually much better than I feared. It revealed 15 more attributes to add to the ignore list, which I've pushed to this PR. Most of these are sl-table and sl-grid related, but there are also 6 new nimble attributes. Between those 4 workspaces I think we will have close to complete coverage. Please give these a look over @jattasNI @rajsite and resolve this if you're satisfied.

Correction, only 3 instances of bad i18n- attributes. I mis-identified one and the other was already fixed.
https://dev.azure.com/ni/DevCentral/_git/Skyline/pullrequest/715355

It's strictly a loosening makes sense, that hadn't computed for me. Also justifies the patch revision 👍

Sounds like part of the discussion for the UI Working Group should be to search for and remove those workarounds and update the list. Might even be tech debt tasks to farm out but that could be a working group discussion. LGTM!

…butes

tests/angular/custom-ignore-attributes/.eslintrc.js

jattasNI · 2024-05-14T16:32:03Z

packages/eslint-config-angular/options.js

+// CSS selector style element[attribute] ignore entries
+// which can be used to scope attribute ignores to specific
+// elements.
+const ignoreAttributes = {


I am not very confident in the completeness of the list. In my innovation day prototype PR I generated it by trial and error on the SystemLinkShared libraries so it could easily be missing APIs that are used by apps (or by nobody yet) if they're not used by a shared library. I think this PR largely uses that list, so it's probably incomplete.

I am fine with making the list comprehensive either before or after completing this PR. I suspect that adopting the package in SLE apps will reveal more APIs than adopting it in the Nimble example app since the latter doesn't exercise APIs very comprehensively. But creating a local package and locally adopting it in every app is a lot of busy work so if @rbell517 would rather wait for Renovate to do that work on a production build and then add any missing attributes as a follow up PR, that's ok by me.

Similar thought on enabling i18n on the example app. I don't object to giving it a shot but I think it'll miss a lot of new APIs.

jattasNI · 2024-05-16T13:39:25Z

This made it into SystemLinkShared with no pipeline failures 🎉

…te/i18n (#143) As a follow up to #140, our original list of attributes was incomplete. I'm adding the attributes that are needed to build Skyline `Web/Workspaces/SystemLinkShared`. [Here's the SLE PR that is blocked on this](https://dev.azure.com/ni/DevCentral/_git/Skyline/pullrequest/716904)

rbell517 requested review from jattasNI, mure, rajsite and TrevorKarjanis as code owners May 3, 2024 17:42

jattasNI reviewed May 6, 2024

View reviewed changes

change/@ni-eslint-config-angular-ff70c9b2-14ea-4eb3-9c0e-0de3cdb388ec.json Outdated Show resolved Hide resolved

Update @angular-eslint/template/i18n to ignore common nimble and sl-l…

0b9ab0a

…ib attributes Change files

rbell517 force-pushed the users/rbell/angular-eslint-i18n-ignore-attributes branch from 80f23f0 to 0b9ab0a Compare May 6, 2024 21:46

rajsite reviewed May 6, 2024

View reviewed changes

packages/eslint-config-angular/i18n-attributes-ignore.js Outdated Show resolved Hide resolved

rajsite reviewed May 6, 2024

View reviewed changes

packages/eslint-config-angular/i18n-attributes-ignore.js Outdated Show resolved Hide resolved

rajsite reviewed May 6, 2024

View reviewed changes

packages/eslint-config-angular/template.js Outdated Show resolved Hide resolved

rbell517 added 2 commits May 10, 2024 14:35

Add new all option and added doc comment to attribute list

68d231a

Fix array expansion.

b62ab66

jattasNI requested a review from rajsite May 10, 2024 19:49

jattasNI approved these changes May 10, 2024

View reviewed changes

Update public api and test support

0c23c75

rajsite reviewed May 10, 2024

View reviewed changes

packages/eslint-config-angular/template.js Show resolved Hide resolved

tests/angular/custom-ignore-attributes/.eslintrc.js Outdated Show resolved Hide resolved

tests/angular/custom-ignore-attributes/.eslintrc.js Show resolved Hide resolved

rajsite added 3 commits May 10, 2024 17:17

Update test comments

e14927d

Align spec test examples

0ff338d

Escape package name

efa00b6

rajsite reviewed May 10, 2024

View reviewed changes

Merge branch 'main' into users/rbell/angular-eslint-i18n-ignore-attri…

6da69a1

…butes

jattasNI approved these changes May 14, 2024

View reviewed changes

TrevorKarjanis approved these changes May 14, 2024

View reviewed changes

Move options path and improve type inference

973fa23

rajsite enabled auto-merge (squash) May 14, 2024 22:29

rajsite disabled auto-merge May 15, 2024 20:59

Add more ignore attributes after examining some workspaces

414fcd6

rajsite approved these changes May 15, 2024

View reviewed changes

rajsite merged commit 060e310 into ni:main May 15, 2024
1 check passed

jattasNI mentioned this pull request May 17, 2024

Add attributes used by SystemLinkShared that shouldn't trigger template/i18n #143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update @angular-eslint/template/i18n to ignore common nimble and sl-lib attributes #140

Update @angular-eslint/template/i18n to ignore common nimble and sl-lib attributes #140

rbell517 commented May 3, 2024 •

edited by rajsite

Loading

jattasNI left a comment

jattasNI left a comment

rajsite May 10, 2024 •

edited

Loading

jattasNI May 14, 2024

rajsite May 14, 2024 •

edited

Loading

rajsite May 14, 2024 •

edited

Loading

rajsite May 15, 2024

rbell517 May 15, 2024 •

edited

Loading

rbell517 May 15, 2024

rajsite May 15, 2024

jattasNI May 14, 2024

jattasNI commented May 16, 2024

Update @angular-eslint/template/i18n to ignore common nimble and sl-lib attributes #140

Update @angular-eslint/template/i18n to ignore common nimble and sl-lib attributes #140

Conversation

rbell517 commented May 3, 2024 • edited by rajsite Loading

Justification

Implementation

Testing

jattasNI left a comment

Choose a reason for hiding this comment

jattasNI left a comment

Choose a reason for hiding this comment

rajsite May 10, 2024 • edited Loading

Choose a reason for hiding this comment

jattasNI May 14, 2024

Choose a reason for hiding this comment

rajsite May 14, 2024 • edited Loading

Choose a reason for hiding this comment

rajsite May 14, 2024 • edited Loading

Choose a reason for hiding this comment

rajsite May 15, 2024

Choose a reason for hiding this comment

rbell517 May 15, 2024 • edited Loading

Choose a reason for hiding this comment

rbell517 May 15, 2024

Choose a reason for hiding this comment

rajsite May 15, 2024

Choose a reason for hiding this comment

jattasNI May 14, 2024

Choose a reason for hiding this comment

jattasNI commented May 16, 2024

rbell517 commented May 3, 2024 •

edited by rajsite

Loading

rajsite May 10, 2024 •

edited

Loading

rajsite May 14, 2024 •

edited

Loading

rajsite May 14, 2024 •

edited

Loading

rbell517 May 15, 2024 •

edited

Loading