[eslint-plugin] Add attribute names rule #4516

justinfagnani · 2024-01-24T00:14:17Z

This PR should be easier to review commit-by-commit. The last commit adds the actual attribute-names rule, the rest of them set things up. I bumped typescript and a few testing dependencies due to package duplication that broke the build while installing some packages.

Type-awareness

This implementation of attribute-names requires a type-checker. This is to detect Lit subclasses and decorators. I think this is a good tradeoff for now, especially as we can support subclasses, ReactiveElement, re-exports, etc (work on that TBD). We almost certainly do want to be able to use different heuristics to detect these things in places where they don't want to run the type-checker, so we can make some detection logic pluggable.

typescript-eslint recommends an option or two different rules in this case:

We recommend against changing rule logic based solely on whether services.program exists. In our experience, users are generally surprised when rules behave differently with or without type information. Additionally, if they misconfigure their ESLint config, they may not realize why the rule started behaving differently. Consider either gating type checking behind an explicit option for the rule or creating two versions of the rule instead.

https://typescript-eslint.io/developers/custom-rules/#typed-rules

We'll have to figure that out.

Copyright assignment

@43081j I originally copied the rule from https://github.com/43081j/eslint-plugin-lit/blob/master/src/rules/attribute-names.ts but ended up rewriting most of it to work against the Lit analyzer. I did copy the tests, then added the correct Lit imports to satisfy the analyzer's detection of LitElement and decorators.

The commits to this repo need to be copyright Google (unless they're a compatible OSS license, credited, and linked). I think the rule implementation qualifies. The test is written by you and you've signed the CLA, so I believe you can just reassign copyright to Google at will with your approval here. Or I can write new tests if necessary.

changeset-bot · 2024-01-24T00:14:21Z

🦋 Changeset detected

Latest commit: 5adce21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@lit-labs/testing	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-01-24T00:14:33Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +2% (-0.82ms - +0.28ms)
^{this-change vs tip-of-tree}

render

this-change: 46.93ms - 48.96ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +6% (-0.11ms - +1.06ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.80ms - +0.57ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: faster ✔ 0% - 7% (0.06ms - 2.36ms)
^{this-change vs tip-of-tree}

update

this-change: 546.72ms - 553.55ms
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +5% (-2.81ms - +2.00ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-1.22ms - +1.75ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-8.04ms - +7.28ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 545.08ms - 550.38ms
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +0% (-12.10ms - +2.38ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6081
Commit: 5adce21

render

Version	Avg time	vs
	46.93ms - 48.96ms	-

update

Version	Avg time	vs
	546.72ms - 553.55ms	-

update-reflect

Version	Avg time	vs
	545.08ms - 550.38ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 121.0.6167.85
Sample size: 30
Built by: Benchmarks #6081
Commit: 5adce21

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.53ms - 19.40ms	-	unsure 🔍 -1% - +6% -0.11ms - +1.06ms	unsure 🔍 -6% - +1% -1.08ms - +0.16ms
tip-of-tree tip-of-tree	18.11ms - 18.87ms	unsure 🔍 -6% - +1% -1.06ms - +0.11ms	-	faster ✔ 2% - 8% 0.35ms - 1.51ms
previous-release previous-release	18.98ms - 19.86ms	unsure 🔍 -1% - +6% -0.16ms - +1.08ms	slower ❌ 2% - 8% 0.35ms - 1.51ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	40.14ms - 43.69ms	-	unsure 🔍 -7% - +5% -2.81ms - +2.00ms	unsure 🔍 -7% - +5% -2.82ms - +2.18ms
tip-of-tree tip-of-tree	40.69ms - 43.95ms	unsure 🔍 -5% - +7% -2.00ms - +2.81ms	-	unsure 🔍 -5% - +6% -2.31ms - +2.49ms
previous-release previous-release	40.47ms - 44.00ms	unsure 🔍 -5% - +7% -2.18ms - +2.82ms	unsure 🔍 -6% - +5% -2.49ms - +2.31ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.88ms - 11.66ms	-	unsure 🔍 -7% - +2% -0.82ms - +0.28ms	unsure 🔍 -6% - +4% -0.63ms - +0.48ms
tip-of-tree tip-of-tree	11.15ms - 11.93ms	unsure 🔍 -3% - +7% -0.28ms - +0.82ms	-	unsure 🔍 -3% - +7% -0.35ms - +0.75ms
previous-release previous-release	10.95ms - 11.74ms	unsure 🔍 -4% - +6% -0.48ms - +0.63ms	unsure 🔍 -6% - +3% -0.75ms - +0.35ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6081
Commit: 5adce21

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.31ms - 34.27ms	-	unsure 🔍 -2% - +2% -0.80ms - +0.57ms	unsure 🔍 -2% - +2% -0.61ms - +0.70ms
tip-of-tree tip-of-tree	33.42ms - 34.40ms	unsure 🔍 -2% - +2% -0.57ms - +0.80ms	-	unsure 🔍 -2% - +2% -0.51ms - +0.83ms
previous-release previous-release	33.30ms - 34.20ms	unsure 🔍 -2% - +2% -0.70ms - +0.61ms	unsure 🔍 -2% - +1% -0.83ms - +0.51ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	73.77ms - 75.88ms	-	unsure 🔍 -2% - +2% -1.22ms - +1.75ms	unsure 🔍 -1% - +3% -0.68ms - +2.00ms
tip-of-tree tip-of-tree	73.52ms - 75.60ms	unsure 🔍 -2% - +2% -1.75ms - +1.22ms	-	unsure 🔍 -1% - +2% -0.94ms - +1.72ms
previous-release previous-release	73.34ms - 75.00ms	unsure 🔍 -3% - +1% -2.00ms - +0.68ms	unsure 🔍 -2% - +1% -1.72ms - +0.94ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6081
Commit: 5adce21

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	30.85ms - 31.69ms	-	faster ✔ 0% - 7% 0.06ms - 2.36ms	unsure 🔍 -3% - +0% -1.05ms - +0.10ms
tip-of-tree tip-of-tree	31.41ms - 33.55ms	slower ❌ 0% - 8% 0.06ms - 2.36ms	-	unsure 🔍 -1% - +6% -0.41ms - +1.87ms
previous-release previous-release	31.35ms - 32.14ms	unsure 🔍 -0% - +3% -0.10ms - +1.05ms	unsure 🔍 -6% - +1% -1.87ms - +0.41ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	526.44ms - 537.86ms	-	unsure 🔍 -2% - +1% -8.04ms - +7.28ms	unsure 🔍 -2% - +1% -9.55ms - +5.77ms
tip-of-tree tip-of-tree	527.42ms - 537.63ms	unsure 🔍 -1% - +2% -7.28ms - +8.04ms	-	unsure 🔍 -2% - +1% -8.73ms - +5.71ms
previous-release previous-release	528.93ms - 539.14ms	unsure 🔍 -1% - +2% -5.77ms - +9.55ms	unsure 🔍 -1% - +2% -5.71ms - +8.73ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	532.03ms - 541.55ms	-	unsure 🔍 -2% - +0% -12.10ms - +2.38ms	unsure 🔍 -2% - +1% -8.36ms - +5.75ms
tip-of-tree tip-of-tree	536.20ms - 547.10ms	unsure 🔍 -0% - +2% -2.38ms - +12.10ms	-	unsure 🔍 -1% - +2% -3.98ms - +11.10ms
previous-release previous-release	532.89ms - 543.30ms	unsure 🔍 -1% - +2% -5.75ms - +8.36ms	unsure 🔍 -2% - +1% -11.10ms - +3.98ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-01-24T00:16:10Z

The size of lit-html.js and lit-core.min.js are as expected.

augustjk

Commenting minor things that stood out to me. I'll defer to other reviewers for the actual rule implementation.
Additionally noting that packages like labs/cli and labs/compiler that have direct dependency on typescript will also need version bumps with this.

packages/labs/eslint-plugin/src/lib/util.ts

packages/labs/eslint-plugin/src/rules/attribute-names.ts

packages/labs/testing/package.json

packages/labs/eslint-plugin/package.json

augustjk · 2024-01-24T00:43:53Z

It is curious that this has size regression for core packages. Could changing the TS version change the emitted JS?

justinfagnani · 2024-01-24T00:59:28Z

It is curious that this has size regression for core packages. Could changing the TS version change the emitted JS?

@augustjk I was wondering the same thing 🤷‍♂️

43081j · 2024-01-24T13:47:57Z

typescript-eslint recommends an option or two different rules in this case

i would have two rules i think, a typed and untyped one (which share some logic). then just publish a typed/untyped recommended config

The test is written by you and you've signed the CLA, so I believe you can just reassign copyright to Google at will with your approval here

sounds good to me.

i'm not too concerned with the copyright, copying things over here, etc. as long as im still able to exist as a maintainer alongside you all (which it seems i am)

augustjk · 2024-01-24T19:01:39Z

package-lock.json

@@ -21614,9 +23667,10 @@
      }
    },
    "node_modules/rollup-plugin-terser/node_modules/terser": {
-      "version": "5.19.2",
+      "version": "5.27.0",


Looks like this terser update might be responsible for the bundle size change. I tried updating typescript alone and it didn't change the output but this branch does have a diff. Diff looks innocuous. Somehow previous version lucked out and didn't require an alias for H, but after update it does.

43081j

looks good to me 🥳

rictic · 2024-01-25T01:13:30Z

packages/labs/eslint-plugin/src/test/rules/attribute-names_test.ts

+          messageId: 'casedPropertyWithoutAttribute',
+        },
+      ],
+    },


Huh, I hadn't seen this rule before. Do we like this rule? Do we want to recommend it?

The casedAttribute message seems good, but the casedPropertyWithoutAttribute doesn't seem great to me.

At least, I see a lot of unproblematic code that would trigger this lint.

https://github.com/43081j/eslint-plugin-lit/blob/master/docs/rules/attribute-names.md

I'm planning on porting all the existing rules to ease migration to the new version. I'm sure there is some overlap with Rune's rules, and some we might want to deprecate going forward. I agree that a lot of perfectly fine code may have a camelCased property name and be making the choice to use the lower-case / case insensitive attribute name and shouldn't be flagged. In those cases we may want additional configuration for this rule, and we may want a migration-oriented configuration and not include this in the recommended set.

I should open an RFC on how the plan for all of this.

rictic · 2024-01-25T01:15:01Z

packages/tests/src/web-test-runner.config.ts

+              '--disable-input-event-activation-protection',
+              '--disable-renderer-backgrounding',
+              '--enable-automation',
+              '--disable-ipc-flooding-protection',


where did these new flags come from?

I found these in a few places that were documenting trying to work around Chrome's background tab throttling. I'm trying to separate and/or revert the changes causing the problems to unblock this PR.

rictic · 2024-01-25T01:15:50Z

packages/labs/eslint-plugin/src/index.ts

-export const rules = {
-  'no-kevin': noKevin as unknown as Rule.RuleModule,
-};
+export const rules = {};


Should this be empty?

Co-authored-by: Augustine Kim <augustinekim@google.com>

justinfagnani requested review from rictic and 43081j January 24, 2024 00:14

justinfagnani requested a review from kevinpschaaf as a code owner January 24, 2024 00:14

augustjk reviewed Jan 24, 2024

View reviewed changes

43081j approved these changes Jan 24, 2024

View reviewed changes

rictic reviewed Jan 25, 2024

View reviewed changes

justinfagnani mentioned this pull request Jan 25, 2024

Bump typescript #4520

Merged

rictic mentioned this pull request Jan 25, 2024

[eslint-plugin] Add option to enable casedAttribute but not casedPropertyWithoutAttribute #4521

Open

1 task

justinfagnani force-pushed the linter-attribute-names-rule branch from 9ec8c61 to 348ee0f Compare January 31, 2024 17:58

justinfagnani and others added 8 commits January 31, 2024 15:24

Add node and optionsNode to ReactiveProperty

acf8d9c

Fix a file extension check

1d160f9

Clarify getPropertyAttribute docs

774ad3b

Remove the fake eslint rules

b4f591e

Add attribute-names rule

32ac961

Update packages/labs/eslint-plugin/src/lib/util.ts

e068d4f

Co-authored-by: Augustine Kim <augustinekim@google.com>

Add changesets

e6bf564

Address feedback

5adce21

justinfagnani force-pushed the linter-attribute-names-rule branch from 348ee0f to 5adce21 Compare January 31, 2024 23:24

justinfagnani merged commit c51bc18 into main Feb 1, 2024
10 checks passed

justinfagnani deleted the linter-attribute-names-rule branch February 1, 2024 00:31

lit-robot mentioned this pull request Mar 11, 2024

Version Packages #4579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eslint-plugin] Add attribute names rule #4516

[eslint-plugin] Add attribute names rule #4516

justinfagnani commented Jan 24, 2024

changeset-bot bot commented Jan 24, 2024 •

edited

github-actions bot commented Jan 24, 2024 •

edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Jan 24, 2024 •

edited

augustjk left a comment

augustjk commented Jan 24, 2024

justinfagnani commented Jan 24, 2024

43081j commented Jan 24, 2024

augustjk Jan 24, 2024

43081j left a comment

rictic Jan 25, 2024 •

edited

justinfagnani Jan 25, 2024

rictic Jan 25, 2024

justinfagnani Jan 25, 2024 •

edited

rictic Jan 25, 2024

justinfagnani Jan 25, 2024

[eslint-plugin] Add attribute names rule #4516

[eslint-plugin] Add attribute names rule #4516

Conversation

justinfagnani commented Jan 24, 2024

Type-awareness

Copyright assignment

changeset-bot bot commented Jan 24, 2024 • edited

🦋 Changeset detected

github-actions bot commented Jan 24, 2024 • edited

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Jan 24, 2024 • edited

augustjk left a comment

Choose a reason for hiding this comment

augustjk commented Jan 24, 2024

justinfagnani commented Jan 24, 2024

43081j commented Jan 24, 2024

augustjk Jan 24, 2024

Choose a reason for hiding this comment

43081j left a comment

Choose a reason for hiding this comment

rictic Jan 25, 2024 • edited

Choose a reason for hiding this comment

justinfagnani Jan 25, 2024

Choose a reason for hiding this comment

rictic Jan 25, 2024

Choose a reason for hiding this comment

justinfagnani Jan 25, 2024 • edited

Choose a reason for hiding this comment

rictic Jan 25, 2024

Choose a reason for hiding this comment

justinfagnani Jan 25, 2024

Choose a reason for hiding this comment

changeset-bot bot commented Jan 24, 2024 •

edited

github-actions bot commented Jan 24, 2024 •

edited

github-actions bot commented Jan 24, 2024 •

edited

rictic Jan 25, 2024 •

edited

justinfagnani Jan 25, 2024 •

edited