Enable noImplicitOverride in our tsconfigs, requiring us to add the `override` modifier on overridden fields #2060

rictic · 2021-08-19T19:17:46Z

This seems like a net win for clarity (more verbiage, but it lets the reader know when a method field has special meaning to the superclass), and it's easy on the code author because TS flags it immediately and the editor plugin offers to add the override keyword for you

changeset-bot · 2021-08-19T19:17:48Z

🦋 Changeset detected

Latest commit: 5d166bb

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 · 2021-08-19T19:18:34Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -18% - +2% (-8.69ms - +1.24ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -2% - +2% (-1.77ms - +2.33ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -5% - +6% (-2.19ms - +2.57ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +7% (-0.46ms - +0.90ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-1.37ms - +1.53ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +0% (-2.51ms - +0.30ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -2% - +0% (-20.86ms - +3.61ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -6% - +7% (-7.41ms - +8.42ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +1% (-10.36ms - +2.93ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +2% (-3.52ms - +2.66ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-8.38ms - +13.48ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -2% - +1% (-17.22ms - +5.38ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +1% (-16.69ms - +8.33ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1758
Commit: c6715db

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	103.20ms - 106.09ms	-	unsure 🔍 -2% - +2% -1.77ms - +2.33ms	faster ✔ 24% - 26% 32.81ms - 37.24ms
tip-of-tree tip-of-tree	102.92ms - 105.82ms	unsure 🔍 -2% - +2% -2.33ms - +1.77ms	-	faster ✔ 24% - 27% 33.08ms - 37.52ms
previous-release previous-release	137.99ms - 141.35ms	slower ❌ 31% - 36% 32.81ms - 37.24ms	slower ❌ 31% - 36% 33.08ms - 37.52ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	846.16ms - 862.38ms	-	unsure 🔍 -2% - +0% -20.86ms - +3.61ms	faster ✔ 7% - 10% 68.07ms - 92.25ms
tip-of-tree tip-of-tree	853.74ms - 872.06ms	unsure 🔍 -0% - +2% -3.61ms - +20.86ms	-	faster ✔ 6% - 9% 58.71ms - 84.35ms
previous-release previous-release	925.46ms - 943.40ms	slower ❌ 8% - 11% 68.07ms - 92.25ms	slower ❌ 7% - 10% 58.71ms - 84.35ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	856.96ms - 872.09ms	-	unsure 🔍 -2% - +1% -17.22ms - +5.38ms	faster ✔ 6% - 9% 60.05ms - 85.05ms
tip-of-tree tip-of-tree	862.05ms - 878.84ms	unsure 🔍 -1% - +2% -5.38ms - +17.22ms	-	faster ✔ 6% - 8% 53.61ms - 79.65ms
previous-release previous-release	927.12ms - 947.03ms	slower ❌ 7% - 10% 60.05ms - 85.05ms	slower ❌ 6% - 9% 53.61ms - 79.65ms	-

lit-html-kitchen-sink

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1758
Commit: c6715db

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	45.01ms - 48.23ms	-	unsure 🔍 -5% - +6% -2.19ms - +2.57ms	faster ✔ 21% - 31% 12.21ms - 19.97ms
tip-of-tree tip-of-tree	44.67ms - 48.19ms	unsure 🔍 -6% - +5% -2.57ms - +2.19ms	-	faster ✔ 21% - 31% 12.34ms - 20.23ms
previous-release previous-release	59.18ms - 66.24ms	slower ❌ 26% - 43% 12.21ms - 19.97ms	slower ❌ 26% - 44% 12.34ms - 20.23ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	123.06ms - 134.30ms	-	unsure 🔍 -6% - +7% -7.41ms - +8.42ms	unsure 🔍 -12% - -0% -17.06ms - +0.28ms
tip-of-tree tip-of-tree	122.60ms - 133.74ms	unsure 🔍 -7% - +6% -8.42ms - +7.41ms	-	faster ✔ 0% - 13% 0.26ms - 17.53ms
previous-release previous-release	130.47ms - 143.67ms	unsure 🔍 -0% - +13% -0.28ms - +17.06ms	slower ❌ 0% - 14% 0.26ms - 17.53ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	40.56ms - 45.90ms	-	unsure 🔍 -18% - +2% -8.69ms - +1.24ms	unsure 🔍 -5% - +18% -1.84ms - +7.16ms
tip-of-tree tip-of-tree	42.77ms - 51.15ms	unsure 🔍 -3% - +20% -1.24ms - +8.69ms	-	slower ❌ 1% - 30% 0.84ms - 11.93ms
previous-release previous-release	36.94ms - 44.20ms	unsure 🔍 -16% - +4% -7.16ms - +1.84ms	faster ✔ 3% - 25% 0.84ms - 11.93ms	-

lit-html-repeat

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1758
Commit: c6715db

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.76ms - 13.99ms	-	unsure 🔍 -3% - +7% -0.46ms - +0.90ms	faster ✔ 7% - 17% 1.09ms - 2.57ms
tip-of-tree tip-of-tree	12.86ms - 13.45ms	unsure 🔍 -7% - +3% -0.90ms - +0.46ms	-	faster ✔ 10% - 17% 1.54ms - 2.57ms
previous-release previous-release	14.79ms - 15.63ms	slower ❌ 8% - 20% 1.09ms - 2.57ms	slower ❌ 11% - 20% 1.54ms - 2.57ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	382.30ms - 391.90ms	-	unsure 🔍 -3% - +1% -10.36ms - +2.93ms	faster ✔ 29% - 31% 156.74ms - 173.28ms
tip-of-tree tip-of-tree	386.22ms - 395.40ms	unsure 🔍 -1% - +3% -2.93ms - +10.36ms	-	faster ✔ 28% - 30% 153.14ms - 169.44ms
previous-release previous-release	545.37ms - 558.84ms	slower ❌ 40% - 45% 156.74ms - 173.28ms	slower ❌ 39% - 44% 153.14ms - 169.44ms	-

lit-html-template-heavy

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1758
Commit: c6715db

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	66.38ms - 68.56ms	-	unsure 🔍 -2% - +2% -1.37ms - +1.53ms	faster ✔ 15% - 18% 11.76ms - 14.64ms
tip-of-tree tip-of-tree	66.43ms - 68.36ms	unsure 🔍 -2% - +2% -1.53ms - +1.37ms	-	faster ✔ 15% - 18% 11.92ms - 14.63ms
previous-release previous-release	79.72ms - 81.62ms	slower ❌ 17% - 22% 11.76ms - 14.64ms	slower ❌ 17% - 22% 11.92ms - 14.63ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	133.41ms - 137.72ms	-	unsure 🔍 -3% - +2% -3.52ms - +2.66ms	faster ✔ 9% - 13% 14.01ms - 19.89ms
tip-of-tree tip-of-tree	133.79ms - 138.21ms	unsure 🔍 -2% - +3% -2.66ms - +3.52ms	-	faster ✔ 9% - 13% 13.54ms - 19.51ms
previous-release previous-release	150.52ms - 154.52ms	slower ❌ 10% - 15% 14.01ms - 19.89ms	slower ❌ 10% - 14% 13.54ms - 19.51ms	-

reactive-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1758
Commit: c6715db

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	70.57ms - 72.47ms	-	unsure 🔍 -3% - +0% -2.51ms - +0.30ms	unsure 🔍 -3% - +1% -1.82ms - +0.66ms
tip-of-tree tip-of-tree	71.60ms - 73.66ms	unsure 🔍 -0% - +4% -0.30ms - +2.51ms	-	unsure 🔍 -1% - +3% -0.77ms - +1.83ms
previous-release previous-release	71.31ms - 72.89ms	unsure 🔍 -1% - +3% -0.66ms - +1.82ms	unsure 🔍 -3% - +1% -1.83ms - +0.77ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	833.11ms - 848.85ms	-	unsure 🔍 -1% - +2% -8.38ms - +13.48ms	unsure 🔍 -1% - +2% -8.83ms - +14.30ms
tip-of-tree tip-of-tree	830.84ms - 846.02ms	unsure 🔍 -2% - +1% -13.48ms - +8.38ms	-	unsure 🔍 -1% - +1% -11.19ms - +11.56ms
previous-release previous-release	829.77ms - 846.72ms	unsure 🔍 -2% - +1% -14.30ms - +8.83ms	unsure 🔍 -1% - +1% -11.56ms - +11.19ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	885.11ms - 902.87ms	-	unsure 🔍 -2% - +1% -16.69ms - +8.33ms	unsure 🔍 -2% - +1% -14.22ms - +11.10ms
tip-of-tree tip-of-tree	889.36ms - 906.98ms	unsure 🔍 -1% - +2% -8.33ms - +16.69ms	-	unsure 🔍 -1% - +2% -9.99ms - +15.23ms
previous-release previous-release	886.53ms - 904.57ms	unsure 🔍 -1% - +2% -11.10ms - +14.22ms	unsure 🔍 -2% - +1% -15.23ms - +9.99ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani

Oops. un-approving since the tests are broken!

…de needed)

rictic · 2021-08-22T20:17:18Z

Tests fixed! PTAL

This undoes the change to the ts-transformer tests in #2060 that enabled standard class field emit, because they are incompatible with Lit. TypeScript 3.5.X changed the default for useDefineForClassFields from false to true when target: ESNext.

rictic requested a review from justinfagnani August 19, 2021 19:17

google-cla bot added the cla: yes label Aug 19, 2021

justinfagnani approved these changes Aug 19, 2021

View reviewed changes

justinfagnani requested changes Aug 19, 2021

View reviewed changes

rictic added 14 commits August 20, 2021 14:56

Update TypeScript

8dcfdf0

Bump prettier

98cbfa4

Enable noImplicitOverride in lit-html

ad37676

Enable noImplicitOverride in lit-element

b2c7eb2

Enable noImplicitOverride in reactive-element

20ef390

Enable noImplicitOverride in labs/task

704d789

Enable noImplicitOverride in labs/react

357ad7b

Enable noImplicitOverride in benchmarks

aa046c9

Enable noImplicitOverride in ssr-client

10ae7e4

Enable noImplicitOverride in labs/scoped-registry-mixin

ebb12e7

Enable noImplicitOverride in labs/motion

4001365

Enable noImplicitOverride in lit-starter-ts

8a2ac9b

Enable noImplicitOverride in labs/ssr

f92dfae

Enable noImplicitOverride in all remaining packages (no changes to co…

c4fe026

…de needed)

rictic force-pushed the override branch from de42d3f to c4fe026 Compare August 20, 2021 22:52

rictic requested a review from justinfagnani August 22, 2021 20:17

bicknellr mentioned this pull request Aug 24, 2021

Update all packages to typescript@^4.3.5. #2088

Closed

justinfagnani approved these changes Aug 24, 2021

View reviewed changes

rictic added 4 commits August 25, 2021 10:43

Merge branch 'main' into override

5f35455

Add changeset.

aeb057a

Merge branch 'override' of github.com:polymer/lit-html into override

7c54c10

Update new ts-transformers tests.

5d166bb

rictic merged commit dddbe0c into main Aug 26, 2021

rictic deleted the override branch August 26, 2021 08:58

abdonrd mentioned this pull request Aug 26, 2021

Use root TypeScript #1498

Closed

aomarks mentioned this pull request Aug 28, 2021

[ts-transformers] Disable standard class fields in tests #2105

Merged

arthurevans mentioned this pull request Oct 1, 2021

[lit-starter-*] starter kits require override which isn't documented #2214

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable noImplicitOverride in our tsconfigs, requiring us to add the `override` modifier on overridden fields #2060

Enable noImplicitOverride in our tsconfigs, requiring us to add the `override` modifier on overridden fields #2060

rictic commented Aug 19, 2021

changeset-bot bot commented Aug 19, 2021 •

edited

github-actions bot commented Aug 19, 2021 •

edited

📊 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

justinfagnani left a comment

rictic commented Aug 22, 2021

Enable noImplicitOverride in our tsconfigs, requiring us to add the override modifier on overridden fields #2060

Enable noImplicitOverride in our tsconfigs, requiring us to add the override modifier on overridden fields #2060

Conversation

rictic commented Aug 19, 2021

changeset-bot bot commented Aug 19, 2021 • edited

🦋 Changeset detected

github-actions bot commented Aug 19, 2021 • edited

📊 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

justinfagnani left a comment

Choose a reason for hiding this comment

rictic commented Aug 22, 2021

Enable noImplicitOverride in our tsconfigs, requiring us to add the `override` modifier on overridden fields #2060

Enable noImplicitOverride in our tsconfigs, requiring us to add the `override` modifier on overridden fields #2060

changeset-bot bot commented Aug 19, 2021 •

edited

github-actions bot commented Aug 19, 2021 •

edited