Skip to content

[@lit-labs/router] Make _ prefixed class members private fixing renaming.#2937

Merged
AndrewJakubowicz merged 2 commits intomainfrom
fix-router-interface
May 24, 2022
Merged

[@lit-labs/router] Make _ prefixed class members private fixing renaming.#2937
AndrewJakubowicz merged 2 commits intomainfrom
fix-router-interface

Conversation

@AndrewJakubowicz
Copy link
Copy Markdown
Contributor

@AndrewJakubowicz AndrewJakubowicz commented May 23, 2022 •
edited
Loading

Addresses: #2925

Context

Currently we have a rollup renaming rule that renames properties prefixed with _.

The renaming is incompatible with subclassing.

Fix

Make the underscored properties private, and expose getters for public/protected endpoints.

For example, _currentParams has a public API: params.

We can add a public API as we better understand what is required.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented May 23, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: cf62a2e

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

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
Copy link
Copy Markdown
Contributor

github-actions bot commented May 23, 2022 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -8% - +6% (-2.63ms - +1.95ms)
    this-change vs tip-of-tree

render

  • lit-element-list: unsure 🔍 -2% - +5% (-1.86ms - +5.15ms)
    this-change vs tip-of-tree
  • lit-html-kitchen-sink: unsure 🔍 -6% - +3% (-2.36ms - +1.16ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -4% - +6% (-0.52ms - +0.80ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -0% - +6% (-0.10ms - +4.10ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -10% - +17% (-7.76ms - +13.10ms)
    this-change vs tip-of-tree

update

  • lit-element-list: unsure 🔍 -5% - +1% (-51.09ms - +11.85ms)
    this-change vs tip-of-tree
  • lit-html-kitchen-sink: unsure 🔍 -6% - +5% (-6.93ms - +5.48ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -7% - +7% (-32.30ms - +29.28ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -3% - +3% (-5.12ms - +4.33ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -9% - +11% (-147.80ms - +181.43ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: unsure 🔍 -2% - +1% (-20.98ms - +12.72ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -2% - +3% (-26.63ms - +38.14ms)
    this-change vs tip-of-tree

Results

lit-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
93.64ms - 99.57ms-unsure 🔍
-2% - +5%
-1.86ms - +5.15ms		faster ✔
17% - 22%
19.84ms - 27.24ms
tip-of-tree
tip-of-tree		93.08ms - 96.83msunsure 🔍
-5% - +2%
-5.15ms - +1.86ms		-faster ✔
19% - 23%
22.28ms - 28.09ms
previous-release
previous-release		117.92ms - 122.36msslower ❌
20% - 29%
19.84ms - 27.24ms		slower ❌
23% - 30%
22.28ms - 28.09ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
1014.62ms - 1043.42ms-unsure 🔍
-5% - +1%
-51.09ms - +11.85ms		faster ✔
33% - 36%
520.01ms - 581.20ms
tip-of-tree
tip-of-tree		1020.66ms - 1076.62msunsure 🔍
-1% - +5%
-11.85ms - +51.09ms		-faster ✔
32% - 36%
492.11ms - 569.87ms
previous-release
previous-release		1552.64ms - 1606.62msslower ❌
50% - 57%
520.01ms - 581.20ms		slower ❌
46% - 55%
492.11ms - 569.87ms		-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
1001.69ms - 1027.64ms-unsure 🔍
-2% - +1%
-20.98ms - +12.72ms		faster ✔
7% - 11%
82.48ms - 127.37ms
tip-of-tree
tip-of-tree		1008.05ms - 1029.55msunsure 🔍
-1% - +2%
-12.72ms - +20.98ms		-faster ✔
7% - 11%
79.56ms - 122.03ms
previous-release
previous-release		1101.28ms - 1137.91msslower ❌
8% - 13%
82.48ms - 127.37ms		slower ❌
8% - 12%
79.56ms - 122.03ms		-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
37.84ms - 39.63ms-unsure 🔍
-6% - +3%
-2.36ms - +1.16ms		faster ✔
11% - 18%
5.01ms - 8.54ms
tip-of-tree
tip-of-tree		37.81ms - 40.85msunsure 🔍
-3% - +6%
-1.16ms - +2.36ms		-faster ✔
9% - 18%
4.02ms - 8.33ms
previous-release
previous-release		43.98ms - 47.03msslower ❌
13% - 22%
5.01ms - 8.54ms		slower ❌
10% - 22%
4.02ms - 8.33ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
108.37ms - 114.70ms-unsure 🔍
-6% - +5%
-6.93ms - +5.48ms		unsure 🔍
-8% - +1%
-9.56ms - +1.39ms
tip-of-tree
tip-of-tree		106.93ms - 117.59msunsure 🔍
-5% - +6%
-5.48ms - +6.93ms		-unsure 🔍
-9% - +3%
-10.32ms - +3.59ms
previous-release
previous-release		111.16ms - 120.09msunsure 🔍
-1% - +9%
-1.39ms - +9.56ms		unsure 🔍
-3% - +9%
-3.59ms - +10.32ms		-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
32.81ms - 36.17ms-unsure 🔍
-8% - +6%
-2.63ms - +1.95ms		faster ✔
7% - 19%
2.43ms - 7.74ms
tip-of-tree
tip-of-tree		33.28ms - 36.39msunsure 🔍
-6% - +8%
-1.95ms - +2.63ms		-faster ✔
6% - 18%
2.16ms - 7.33ms
previous-release
previous-release		37.52ms - 41.64msslower ❌
7% - 23%
2.43ms - 7.74ms		slower ❌
6% - 21%
2.16ms - 7.33ms		-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
13.30ms - 14.37ms-unsure 🔍
-4% - +6%
-0.52ms - +0.80ms		faster ✔
5% - 17%
0.70ms - 2.68ms
tip-of-tree
tip-of-tree		13.31ms - 14.08msunsure 🔍
-6% - +4%
-0.80ms - +0.52ms		-faster ✔
6% - 17%
0.91ms - 2.75ms
previous-release
previous-release		14.69ms - 16.36msslower ❌
5% - 20%
0.70ms - 2.68ms		slower ❌
7% - 20%
0.91ms - 2.75ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
420.26ms - 463.39ms-unsure 🔍
-7% - +7%
-32.30ms - +29.28ms		faster ✔
21% - 30%
120.83ms - 182.32ms
tip-of-tree
tip-of-tree		421.35ms - 465.31msunsure 🔍
-7% - +7%
-29.28ms - +32.30ms		-faster ✔
21% - 30%
119.03ms - 181.11ms
previous-release
previous-release		571.49ms - 615.32msslower ❌
26% - 43%
120.83ms - 182.32ms		slower ❌
26% - 42%
119.03ms - 181.11ms		-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
69.74ms - 73.08ms-unsure 🔍
-0% - +6%
-0.10ms - +4.10ms		faster ✔
14% - 19%
11.53ms - 16.58ms
tip-of-tree
tip-of-tree		68.14ms - 70.68msunsure 🔍
-6% - +0%
-4.10ms - +0.10ms		-faster ✔
16% - 21%
13.77ms - 18.34ms
previous-release
previous-release		83.57ms - 87.37msslower ❌
16% - 24%
11.53ms - 16.58ms		slower ❌
20% - 27%
13.77ms - 18.34ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
159.22ms - 165.28ms-unsure 🔍
-3% - +3%
-5.12ms - +4.33ms		faster ✔
11% - 15%
19.86ms - 29.15ms
tip-of-tree
tip-of-tree		159.01ms - 166.26msunsure 🔍
-3% - +3%
-4.33ms - +5.12ms		-faster ✔
10% - 15%
19.06ms - 29.17ms
previous-release
previous-release		183.23ms - 190.27msslower ❌
12% - 18%
19.86ms - 29.15ms		slower ❌
11% - 18%
19.06ms - 29.17ms		-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
74.21ms - 89.94ms-unsure 🔍
-10% - +17%
-7.76ms - +13.10ms		unsure 🔍
-3% - +25%
-2.01ms - +18.02ms
tip-of-tree
tip-of-tree		72.56ms - 86.25msunsure 🔍
-16% - +9%
-13.10ms - +7.76ms		-unsure 🔍
-6% - +20%
-3.90ms - +14.57ms
previous-release
previous-release		67.87ms - 80.27msunsure 🔍
-21% - +2%
-18.02ms - +2.01ms		unsure 🔍
-18% - +4%
-14.57ms - +3.90ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
1546.15ms - 1778.46ms-unsure 🔍
-9% - +11%
-147.80ms - +181.43ms		unsure 🔍
-11% - +8%
-184.61ms - +137.70ms
tip-of-tree
tip-of-tree		1528.84ms - 1762.14msunsure 🔍
-11% - +9%
-181.43ms - +147.80ms		-unsure 🔍
-12% - +7%
-201.79ms - +121.25ms
previous-release
previous-release		1574.05ms - 1797.48msunsure 🔍
-8% - +11%
-137.70ms - +184.61ms		unsure 🔍
-7% - +12%
-121.25ms - +201.79ms		-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
1124.58ms - 1167.63ms-unsure 🔍
-2% - +3%
-26.63ms - +38.14ms		unsure 🔍
-3% - +2%
-34.94ms - +26.01ms
tip-of-tree
tip-of-tree		1116.15ms - 1164.54msunsure 🔍
-3% - +2%
-38.14ms - +26.63ms		-unsure 🔍
-4% - +2%
-42.64ms - +22.19ms
previous-release
previous-release		1129.00ms - 1172.14msunsure 🔍
-2% - +3%
-26.01ms - +34.94ms		unsure 🔍
-2% - +4%
-22.19ms - +42.64ms		-

tachometer-reporter-action v2 for Benchmarks

@AndrewJakubowicz AndrewJakubowicz marked this pull request as ready for review May 23, 2022 20:20
justinfagnani
justinfagnani approved these changes May 23, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might also have some work to do here to use a prefix for renaming to avoid accidental name collisions, or prevent subclassing.

@AndrewJakubowicz AndrewJakubowicz merged commit d2584ad into main May 24, 2022
@AndrewJakubowicz AndrewJakubowicz deleted the fix-router-interface branch May 24, 2022 18:20
@lit-robot lit-robot mentioned this pull request May 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@justinfagnani justinfagnani justinfagnani approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AndrewJakubowicz @justinfagnani