[labs/analyzer] Handle class private fields in LitElement analysis #4746

justinfagnani · 2024-08-22T17:12:22Z

This change allows private fields to be handled, but because they're not likely to be reactive properties, they're effectively ignored in this code path.

changeset-bot · 2024-08-22T17:12:26Z

🦋 Changeset detected

Latest commit: 9b67740

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/analyzer	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-08-22T17:12:38Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +8% (-0.17ms - +0.91ms)
^{this-change vs tip-of-tree}

render

this-change: 44.51ms - 45.98ms
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +3% (-0.70ms - +0.64ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +2% (-0.86ms - +0.55ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +2% (-1.69ms - +0.55ms)
^{this-change vs tip-of-tree}

update

this-change: 470.22ms - 474.35ms
this-change, tip-of-tree, previous-release: slower ❌ 0% - 12% (0.13ms - 4.47ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-0.35ms - +0.99ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: faster ✔ 0% - 1% (0.18ms - 4.24ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 472.35ms - 476.46ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +0% (-5.33ms - +0.79ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 127.0.6533.119
Sample size: 50
Built by: Benchmarks #6370
Commit: 9b67740

render

Version	Avg time	vs
	44.51ms - 45.98ms	-

update

Version	Avg time	vs
	470.22ms - 474.35ms	-

update-reflect

Version	Avg time	vs
	472.35ms - 476.46ms	-

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

Browser: chrome-headless 127.0.6533.119
Sample size: 30
Built by: Benchmarks #6370
Commit: 9b67740

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.17ms - 19.03ms	-	unsure 🔍 -4% - +3% -0.70ms - +0.64ms	unsure 🔍 -3% - +4% -0.49ms - +0.67ms
tip-of-tree tip-of-tree	18.11ms - 19.14ms	unsure 🔍 -3% - +4% -0.64ms - +0.70ms	-	unsure 🔍 -3% - +4% -0.53ms - +0.76ms
previous-release previous-release	18.13ms - 18.89ms	unsure 🔍 -4% - +3% -0.67ms - +0.49ms	unsure 🔍 -4% - +3% -0.76ms - +0.53ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	37.14ms - 40.23ms	-	slower ❌ 0% - 12% 0.13ms - 4.47ms	unsure 🔍 -2% - +10% -0.74ms - +3.66ms
tip-of-tree tip-of-tree	34.87ms - 37.91ms	faster ✔ 0% - 11% 0.13ms - 4.47ms	-	unsure 🔍 -8% - +4% -3.02ms - +1.35ms
previous-release previous-release	35.66ms - 38.79ms	unsure 🔍 -9% - +2% -3.66ms - +0.74ms	unsure 🔍 -4% - +8% -1.35ms - +3.02ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.75ms - 12.35ms	-	unsure 🔍 -2% - +8% -0.17ms - +0.91ms	unsure 🔍 -3% - +4% -0.32ms - +0.49ms
tip-of-tree tip-of-tree	11.23ms - 12.13ms	unsure 🔍 -8% - +1% -0.91ms - +0.17ms	-	unsure 🔍 -7% - +2% -0.81ms - +0.24ms
previous-release previous-release	11.69ms - 12.24ms	unsure 🔍 -4% - +3% -0.49ms - +0.32ms	unsure 🔍 -2% - +7% -0.24ms - +0.81ms	-

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

Browser: chrome-headless 127.0.6533.119
Sample size: 50
Built by: Benchmarks #6370
Commit: 9b67740

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.07ms - 33.70ms	-	unsure 🔍 -3% - +2% -0.86ms - +0.55ms	unsure 🔍 -1% - +2% -0.32ms - +0.56ms
tip-of-tree tip-of-tree	32.91ms - 34.17ms	unsure 🔍 -2% - +3% -0.55ms - +0.86ms	-	unsure 🔍 -1% - +3% -0.43ms - +0.98ms
previous-release previous-release	32.96ms - 33.58ms	unsure 🔍 -2% - +1% -0.56ms - +0.32ms	unsure 🔍 -3% - +1% -0.98ms - +0.43ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	66.45ms - 67.51ms	-	unsure 🔍 -1% - +1% -0.35ms - +0.99ms	unsure 🔍 -2% - +1% -1.42ms - +0.64ms
tip-of-tree tip-of-tree	66.25ms - 67.07ms	unsure 🔍 -1% - +1% -0.99ms - +0.35ms	-	unsure 🔍 -2% - +0% -1.69ms - +0.26ms
previous-release previous-release	66.49ms - 68.26ms	unsure 🔍 -1% - +2% -0.64ms - +1.42ms	unsure 🔍 -0% - +3% -0.26ms - +1.69ms	-

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

Browser: chrome-headless 127.0.6533.119
Sample size: 50
Built by: Benchmarks #6370
Commit: 9b67740

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.18ms - 32.02ms	-	unsure 🔍 -5% - +2% -1.69ms - +0.55ms	unsure 🔍 -1% - +2% -0.42ms - +0.75ms
tip-of-tree tip-of-tree	31.13ms - 33.21ms	unsure 🔍 -2% - +5% -0.55ms - +1.69ms	-	unsure 🔍 -1% - +6% -0.39ms - +1.85ms
previous-release previous-release	31.02ms - 31.85ms	unsure 🔍 -2% - +1% -0.75ms - +0.42ms	unsure 🔍 -6% - +1% -1.85ms - +0.39ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	476.52ms - 479.31ms	-	faster ✔ 0% - 1% 0.18ms - 4.24ms	unsure 🔍 -1% - -0% -4.20ms - +0.00ms
tip-of-tree tip-of-tree	478.65ms - 481.60ms	slower ❌ 0% - 1% 0.18ms - 4.24ms	-	unsure 🔍 -0% - +0% -2.05ms - +2.26ms
previous-release previous-release	478.44ms - 481.59ms	unsure 🔍 -0% - +1% -0.00ms - +4.20ms	unsure 🔍 -0% - +0% -2.26ms - +2.05ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	512.01ms - 515.26ms	-	unsure 🔍 -1% - +0% -5.33ms - +0.79ms	unsure 🔍 -1% - +0% -3.63ms - +0.80ms
tip-of-tree tip-of-tree	513.30ms - 518.50ms	unsure 🔍 -0% - +1% -0.79ms - +5.33ms	-	unsure 🔍 -0% - +1% -2.15ms - +3.85ms
previous-release previous-release	513.54ms - 516.56ms	unsure 🔍 -0% - +1% -0.80ms - +3.63ms	unsure 🔍 -1% - +0% -3.85ms - +2.15ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-08-22T17:14:43Z

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

justinfagnani · 2024-08-22T17:15:57Z

packages/labs/analyzer/src/lib/lit-element/properties.ts


  for (const prop of propertyDeclarations) {
-    if (!ts.isIdentifier(prop.name)) {
+    if (!ts.isIdentifier(prop.name) && !ts.isPrivateIdentifier(prop.name)) {


@kevinpschaaf questions on this function, or you remember: it's named getProperties(), but only returns the reactive properties. 1) Should it be renamed? 2) Since reactive properties are kind of an implementation detail, and a getter/setter pair or different decorator could make a property reactive... are we handling those cases, or should we even distinguish reactive properties from others? Is this to do something else like find attributes?

Yeah, iirc this was properties in the @property sense? The term field is used in the code to refer to generic fields. But yeah could rename to getReactiveProperties().

I think these would be used to determine the attributes for e.g. CEM (I see there's still a TODO in there), but also in the Angular wrapper to wrap them with @Input().

packages/labs/analyzer/test-files/ts/properties/src/element-a.ts

justinfagnani requested a review from aomarks August 22, 2024 17:12

justinfagnani requested a review from kevinpschaaf as a code owner August 22, 2024 17:12

justinfagnani force-pushed the analyzer-private-fields branch from 74d318f to 22cbf71 Compare August 22, 2024 17:13

[labs/analyzer] Handle class private fields in LitElement analysis

c9c5011

justinfagnani force-pushed the analyzer-private-fields branch from 22cbf71 to c9c5011 Compare August 22, 2024 17:13

justinfagnani commented Aug 22, 2024

View reviewed changes

aomarks approved these changes Aug 22, 2024

View reviewed changes

packages/labs/analyzer/test-files/ts/properties/src/element-a.ts Show resolved Hide resolved

Fix tests by making dignostics dedupable

9b67740

justinfagnani merged commit bc3ba55 into main Aug 23, 2024

justinfagnani deleted the analyzer-private-fields branch August 23, 2024 17:16

lit-robot mentioned this pull request Oct 3, 2024

Version Packages #4784

Merged

[labs/analyzer] Handle class private fields in LitElement analysis #4746

[labs/analyzer] Handle class private fields in LitElement analysis #4746

Uh oh!

Conversation

justinfagnani commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Aug 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 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

Uh oh!

github-actions bot commented Aug 22, 2024

Uh oh!

justinfagnani Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinfagnani commented Aug 22, 2024 •

edited

Loading

changeset-bot bot commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading