[labs/analyzer] Fix support for static class members. by kevinpschaaf · Pull Request #3648 · lit/lit

kevinpschaaf · 2023-02-06T19:25:37Z

We were previously storing class members (fields & methods) in a map by name (since field-lookup by name is expected to be a key use case), however this ignored the fact that static and non-static members would conflict.

This PR separates the storage into separate maps, and adds an isStatic optional argument to getField() and getMethod().

changeset-bot · 2023-02-06T19:25:40Z

🦋 Changeset detected

Latest commit: 5531665

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

This PR includes changesets to release 7 packages

Name	Type
@lit-labs/analyzer	Minor
@lit-labs/cli	Patch
@lit-labs/gen-manifest	Patch
@lit-labs/gen-utils	Patch
@lit-labs/gen-wrapper-angular	Patch
@lit-labs/gen-wrapper-react	Patch
@lit-labs/gen-wrapper-vue	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 · 2023-02-06T19:25:56Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -9% - +10% (-2.14ms - +2.42ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 77.07ms - 81.97ms
lit-html-kitchen-sink: unsure 🔍 -6% - +5% (-2.00ms - +1.56ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +5% (-0.41ms - +0.60ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: faster ✔ 1% - 5% (0.60ms - 2.90ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +1% (-1.71ms - +0.55ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 782.13ms - 795.43ms
lit-html-kitchen-sink: unsure 🔍 -8% - +4% (-6.69ms - +3.55ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -0% - +4% (-1.24ms - +11.08ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -4% - +0% (-4.35ms - +0.03ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +0% (-14.72ms - +2.28ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 784.65ms - 793.54ms
reactive-element-list: unsure 🔍 -1% - +0% (-10.93ms - +2.64ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 109.0.5414.119
Sample size: 50
Built by: Benchmarks #4397
Commit: a65254f

render

Version	Avg time	vs
	77.07ms - 81.97ms	-

update

Version	Avg time	vs
	782.13ms - 795.43ms	-

update-reflect

Version	Avg time	vs
	784.65ms - 793.54ms	-

lit-html-kitchen-sink

Browser: chrome-headless 109.0.5414.119
Sample size: 30
Built by: Benchmarks #4397
Commit: a65254f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.11ms - 33.73ms	-	unsure 🔍 -6% - +5% -2.00ms - +1.56ms	unsure 🔍 -3% - +8% -0.88ms - +2.46ms
tip-of-tree tip-of-tree	31.43ms - 33.85ms	unsure 🔍 -5% - +6% -1.56ms - +2.00ms	-	unsure 🔍 -2% - +8% -0.58ms - +2.60ms
previous-release previous-release	30.59ms - 32.67ms	unsure 🔍 -8% - +3% -2.46ms - +0.88ms	unsure 🔍 -8% - +2% -2.60ms - +0.58ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	78.76ms - 83.34ms	-	unsure 🔍 -8% - +4% -6.69ms - +3.55ms	unsure 🔍 -6% - +1% -5.42ms - +1.24ms
tip-of-tree tip-of-tree	78.05ms - 87.21ms	unsure 🔍 -4% - +8% -3.55ms - +6.69ms	-	unsure 🔍 -7% - +6% -5.69ms - +4.66ms
previous-release previous-release	80.73ms - 85.56ms	unsure 🔍 -2% - +7% -1.24ms - +5.42ms	unsure 🔍 -6% - +7% -4.66ms - +5.69ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	23.14ms - 26.43ms	-	unsure 🔍 -9% - +10% -2.14ms - +2.42ms	unsure 🔍 -13% - +6% -3.50ms - +1.53ms
tip-of-tree tip-of-tree	23.06ms - 26.23ms	unsure 🔍 -10% - +9% -2.42ms - +2.14ms	-	unsure 🔍 -14% - +5% -3.61ms - +1.35ms
previous-release previous-release	23.86ms - 27.68ms	unsure 🔍 -6% - +14% -1.53ms - +3.50ms	unsure 🔍 -6% - +15% -1.35ms - +3.61ms	-

lit-html-repeat

Browser: chrome-headless 109.0.5414.119
Sample size: 30
Built by: Benchmarks #4397
Commit: a65254f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.62ms - 12.32ms	-	unsure 🔍 -3% - +5% -0.41ms - +0.60ms	unsure 🔍 -4% - +5% -0.43ms - +0.59ms
tip-of-tree tip-of-tree	11.51ms - 12.24ms	unsure 🔍 -5% - +3% -0.60ms - +0.41ms	-	unsure 🔍 -4% - +4% -0.53ms - +0.50ms
previous-release previous-release	11.52ms - 12.26ms	unsure 🔍 -5% - +4% -0.59ms - +0.43ms	unsure 🔍 -4% - +4% -0.50ms - +0.53ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	305.74ms - 315.37ms	-	unsure 🔍 -0% - +4% -1.24ms - +11.08ms	unsure 🔍 -2% - +2% -5.81ms - +6.89ms
tip-of-tree tip-of-tree	301.80ms - 309.48ms	unsure 🔍 -4% - +0% -11.08ms - +1.24ms	-	unsure 🔍 -3% - +0% -10.02ms - +1.26ms
previous-release previous-release	305.88ms - 314.15ms	unsure 🔍 -2% - +2% -6.89ms - +5.81ms	unsure 🔍 -0% - +3% -1.26ms - +10.02ms	-

lit-html-template-heavy

Browser: chrome-headless 109.0.5414.119
Sample size: 50
Built by: Benchmarks #4397
Commit: a65254f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.04ms - 54.43ms	-	faster ✔ 1% - 5% 0.60ms - 2.90ms	unsure 🔍 -3% - +1% -1.55ms - +0.28ms
tip-of-tree tip-of-tree	54.56ms - 56.40ms	slower ❌ 1% - 5% 0.60ms - 2.90ms	-	slower ❌ 0% - 4% 0.02ms - 2.21ms
previous-release previous-release	53.77ms - 54.97ms	unsure 🔍 -1% - +3% -0.28ms - +1.55ms	faster ✔ 0% - 4% 0.02ms - 2.21ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	116.82ms - 120.03ms	-	unsure 🔍 -4% - +0% -4.35ms - +0.03ms	unsure 🔍 -2% - +2% -2.44ms - +1.86ms
tip-of-tree tip-of-tree	119.09ms - 122.07ms	unsure 🔍 -0% - +4% -0.03ms - +4.35ms	-	unsure 🔍 -0% - +3% -0.20ms - +3.93ms
previous-release previous-release	117.28ms - 120.15ms	unsure 🔍 -2% - +2% -1.86ms - +2.44ms	unsure 🔍 -3% - +0% -3.93ms - +0.20ms	-

reactive-element-list

Browser: chrome-headless 109.0.5414.119
Sample size: 50
Built by: Benchmarks #4397
Commit: a65254f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.48ms - 53.01ms	-	unsure 🔍 -3% - +1% -1.71ms - +0.55ms	unsure 🔍 -0% - +4% -0.08ms - +1.97ms
tip-of-tree tip-of-tree	51.99ms - 53.66ms	unsure 🔍 -1% - +3% -0.55ms - +1.71ms	-	slower ❌ 1% - 5% 0.45ms - 2.60ms
previous-release previous-release	50.62ms - 51.99ms	unsure 🔍 -4% - +0% -1.97ms - +0.08ms	faster ✔ 1% - 5% 0.45ms - 2.60ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	808.39ms - 820.18ms	-	unsure 🔍 -2% - +0% -14.72ms - +2.28ms	unsure 🔍 -1% - +1% -10.53ms - +6.38ms
tip-of-tree tip-of-tree	814.38ms - 826.63ms	unsure 🔍 -0% - +2% -2.28ms - +14.72ms	-	unsure 🔍 -1% - +2% -4.48ms - +12.77ms
previous-release previous-release	810.29ms - 822.43ms	unsure 🔍 -1% - +1% -6.38ms - +10.53ms	unsure 🔍 -2% - +1% -12.77ms - +4.48ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	827.50ms - 836.59ms	-	unsure 🔍 -1% - +0% -10.93ms - +2.64ms	unsure 🔍 -1% - +1% -7.69ms - +5.58ms
tip-of-tree tip-of-tree	831.15ms - 841.22ms	unsure 🔍 -0% - +1% -2.64ms - +10.93ms	-	unsure 🔍 -0% - +1% -3.89ms - +10.06ms
previous-release previous-release	828.27ms - 837.93ms	unsure 🔍 -1% - +1% -5.58ms - +7.69ms	unsure 🔍 -1% - +0% -10.06ms - +3.89ms	-

_{tachometer-reporter-action v2 for Benchmarks}

bicknellr

WDYT about splitting the four getters into separate ones for static vs. member? I get the feeling that it's more likely that a user would care about only one group at a time since they represent properties of different objects, which might avoid extra (e.g.) classDecl.fields.filter(x => !x.isStatic).

bicknellr · 2023-02-06T23:01:31Z

packages/labs/analyzer/src/lib/model.ts

   */
-  get methods(): IterableIterator<ClassMethod> {
-    return this._methodMap.values();
+  get methods() {


Does removing this return type cause it to implicitly be Array<ClassMethod>? Is that something we care about w.r.t. API stability / flexibility? (vs. the original return type, which is more generic?)

It does, and I don't think we care that much about stability at this exact point. It's not possible to make a generator getter directly (where we could just yield both values() out; it would need to call out to one), and didn't figure it was worth the trouble to keep this an iterator.

Oh, separating them allows keeping that return type I guess. Might as well.

kevinpschaaf · 2023-02-07T00:32:09Z

WDYT about splitting the four getters into separate ones for static vs. member? I get the feeling that it's more likely that a user would care about only one group at a time since they represent properties of different objects, which might avoid extra (e.g.) classDecl.fields.filter(x => !x.isStatic).

Yeah, that's a good point. It's likely a documentation generator wants to ensure the static and non-static members are collated, so it'd need to do just that I think. I'll separate them.

justinfagnani · 2023-02-07T00:57:43Z

packages/labs/analyzer/src/lib/model.ts

   */
-  getField(name: string): ClassField | undefined {
-    return this._fieldMap.get(name);
+  getField(name: string, isStatic = false): ClassField | undefined {


Given that we have fields and staticFields, I'm not sure what value there is in a parameter to separate the two here. I would default to mirroring the two APIs and making getStaticField(), etc.

Fix support for static class members.

c58f2b3

kevinpschaaf requested a review from justinfagnani as a code owner February 6, 2023 19:25

Update golden.

442ecf9

kevinpschaaf requested a review from bicknellr February 6, 2023 19:27

bicknellr reviewed Feb 6, 2023

View reviewed changes

Address feedback: add separate static member getters

18f83f2

kevinpschaaf requested a review from bicknellr February 7, 2023 00:37

bicknellr approved these changes Feb 7, 2023

View reviewed changes

justinfagnani approved these changes Feb 7, 2023

View reviewed changes

Separate class member get-by-name methods by static/non-static

5531665

kevinpschaaf enabled auto-merge (squash) February 7, 2023 02:00

kevinpschaaf merged commit 39ac527 into main Feb 7, 2023

kevinpschaaf deleted the analyzer-static branch February 7, 2023 02:06

lit-robot mentioned this pull request Mar 10, 2023

Version Packages #3717

Merged

Conversation

kevinpschaaf commented Feb 6, 2023

Uh oh!

changeset-bot bot commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Feb 6, 2023 • 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

render

update

update-reflect

Uh oh!

bicknellr left a comment

Choose a reason for hiding this comment

Uh oh!

bicknellr Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf commented Feb 7, 2023

Uh oh!

justinfagnani Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Feb 6, 2023 •

edited

Loading

github-actions bot commented Feb 6, 2023 •

edited

Loading

bicknellr Feb 6, 2023 •

edited

Loading