Correct polyfill-support styling issues #2002

sorvell · 2021-07-13T20:57:35Z

Fixes #2000: ShadyCSS's styleElement is now called before firstUpdate/update so that code in these methods can properly assume styling (including shimmed custom properties) has fully applied.

Fixes #2001: Late added styles are now retained in templates. This behavior is consistent with what happened in Lit 1 and we're electing not to change it so that users that may be relying on this (admittedly weird and not well supported) behavior are not broken.

Fixes #2000: ShadyCSS's styleElement is now called before firstUpdate/update so that code in these methods can properly assume styling (including shimmed custom properties) has fully applied. Fixes #2001: Late added styles are now retained in templates. This behavior is consistent with what happened in Lit 1 and we're electing not to change it so that users that may be relying on this (admittedly weird and not well supported) behavior are not broken.

changeset-bot · 2021-07-13T20:57:37Z

🦋 Changeset detected

Latest commit: 5251e3b

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-07-13T20:58:02Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -5% - +2% (-1.78ms - +0.75ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -1% - +3% (-0.59ms - +2.86ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -1% - +4% (-0.41ms - +1.56ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +2% (-0.42ms - +0.23ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +1% (-0.95ms - +0.97ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-0.71ms - +1.51ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -1% - +1% (-8.78ms - +9.94ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -4% - +3% (-3.56ms - +2.67ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -2% - +2% (-6.76ms - +6.02ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +1% (-4.31ms - +1.17ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +1% (-14.65ms - +5.33ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -2% - +0% (-15.36ms - +3.51ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-6.85ms - +15.49ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 91.0.4472.114
Sample size: 50
Built by: Benchmarks #1593
Commit: e5f0b51

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	99.91ms - 102.02ms	-	unsure 🔍 -1% - +3% -0.59ms - +2.86ms	faster ✔ 23% - 25% 30.83ms - 34.19ms
tip-of-tree tip-of-tree	98.46ms - 101.19ms	unsure 🔍 -3% - +1% -2.86ms - +0.59ms	-	faster ✔ 24% - 26% 31.76ms - 35.54ms
previous-release previous-release	132.17ms - 134.78ms	slower ❌ 30% - 34% 30.83ms - 34.19ms	slower ❌ 31% - 36% 31.76ms - 35.54ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	839.03ms - 851.17ms	-	unsure 🔍 -1% - +1% -8.78ms - +9.94ms	faster ✔ 7% - 8% 60.88ms - 77.73ms
tip-of-tree tip-of-tree	837.39ms - 851.65ms	unsure 🔍 -1% - +1% -9.94ms - +8.78ms	-	faster ✔ 7% - 9% 60.66ms - 79.11ms
previous-release previous-release	908.55ms - 920.26ms	slower ❌ 7% - 9% 60.88ms - 77.73ms	slower ❌ 7% - 9% 60.66ms - 79.11ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	870.97ms - 883.75ms	-	unsure 🔍 -2% - +0% -15.36ms - +3.51ms	faster ✔ 5% - 7% 41.92ms - 60.84ms
tip-of-tree tip-of-tree	876.34ms - 890.23ms	unsure 🔍 -0% - +2% -3.51ms - +15.36ms	-	faster ✔ 4% - 6% 35.61ms - 55.30ms
previous-release previous-release	921.76ms - 935.72ms	slower ❌ 5% - 7% 41.92ms - 60.84ms	slower ❌ 4% - 6% 35.61ms - 55.30ms	-

lit-html-kitchen-sink

Browser: chrome-headless 91.0.4472.114
Sample size: 30
Built by: Benchmarks #1593
Commit: e5f0b51

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	41.79ms - 43.49ms	-	unsure 🔍 -1% - +4% -0.41ms - +1.56ms	faster ✔ 13% - 18% 6.73ms - 9.16ms
tip-of-tree tip-of-tree	41.57ms - 42.55ms	unsure 🔍 -4% - +1% -1.56ms - +0.41ms	-	faster ✔ 15% - 19% 7.52ms - 9.52ms
previous-release previous-release	49.71ms - 51.45ms	slower ❌ 16% - 22% 6.73ms - 9.16ms	slower ❌ 18% - 23% 7.52ms - 9.52ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	98.32ms - 103.48ms	-	unsure 🔍 -4% - +3% -3.56ms - +2.67ms	unsure 🔍 -5% - +1% -5.65ms - +1.49ms
tip-of-tree tip-of-tree	99.60ms - 103.09ms	unsure 🔍 -3% - +4% -2.67ms - +3.56ms	-	unsure 🔍 -4% - +1% -4.66ms - +1.39ms
previous-release previous-release	100.52ms - 105.45ms	unsure 🔍 -2% - +6% -1.49ms - +5.65ms	unsure 🔍 -1% - +5% -1.39ms - +4.66ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	35.31ms - 36.73ms	-	unsure 🔍 -5% - +2% -1.78ms - +0.75ms	unsure 🔍 -2% - +5% -0.55ms - +1.76ms
tip-of-tree tip-of-tree	35.49ms - 37.59ms	unsure 🔍 -2% - +5% -0.75ms - +1.78ms	-	unsure 🔍 -1% - +7% -0.27ms - +2.51ms
previous-release previous-release	34.51ms - 36.32ms	unsure 🔍 -5% - +2% -1.76ms - +0.55ms	unsure 🔍 -7% - +1% -2.51ms - +0.27ms	-

lit-html-repeat

Browser: chrome-headless 91.0.4472.114
Sample size: 30
Built by: Benchmarks #1593
Commit: e5f0b51

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	13.05ms - 13.45ms	-	unsure 🔍 -3% - +2% -0.42ms - +0.23ms	faster ✔ 11% - 15% 1.64ms - 2.33ms
tip-of-tree tip-of-tree	13.10ms - 13.60ms	unsure 🔍 -2% - +3% -0.23ms - +0.42ms	-	faster ✔ 10% - 15% 1.52ms - 2.26ms
previous-release previous-release	14.96ms - 15.51ms	slower ❌ 12% - 18% 1.64ms - 2.33ms	slower ❌ 11% - 17% 1.52ms - 2.26ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	389.54ms - 399.09ms	-	unsure 🔍 -2% - +2% -6.76ms - +6.02ms	faster ✔ 28% - 31% 156.00ms - 173.26ms
tip-of-tree tip-of-tree	390.44ms - 398.93ms	unsure 🔍 -2% - +2% -6.02ms - +6.76ms	-	faster ✔ 28% - 31% 155.91ms - 172.61ms
previous-release previous-release	551.76ms - 566.14ms	slower ❌ 39% - 44% 156.00ms - 173.26ms	slower ❌ 39% - 44% 155.91ms - 172.61ms	-

lit-html-template-heavy

Browser: chrome-headless 91.0.4472.114
Sample size: 50
Built by: Benchmarks #1593
Commit: e5f0b51

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	65.43ms - 66.89ms	-	unsure 🔍 -1% - +1% -0.95ms - +0.97ms	faster ✔ 16% - 20% 12.62ms - 15.98ms
tip-of-tree tip-of-tree	65.53ms - 66.78ms	unsure 🔍 -1% - +1% -0.97ms - +0.95ms	-	faster ✔ 16% - 20% 12.67ms - 15.94ms
previous-release previous-release	78.94ms - 81.97ms	slower ❌ 19% - 24% 12.62ms - 15.98ms	slower ❌ 19% - 24% 12.67ms - 15.94ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	130.23ms - 133.98ms	-	unsure 🔍 -3% - +1% -4.31ms - +1.17ms	faster ✔ 12% - 16% 18.25ms - 24.59ms
tip-of-tree tip-of-tree	131.68ms - 135.66ms	unsure 🔍 -1% - +3% -1.17ms - +4.31ms	-	faster ✔ 11% - 15% 16.61ms - 23.09ms
previous-release previous-release	150.96ms - 156.08ms	slower ❌ 14% - 19% 18.25ms - 24.59ms	slower ❌ 12% - 17% 16.61ms - 23.09ms	-

reactive-element-list

Browser: chrome-headless 91.0.4472.114
Sample size: 50
Built by: Benchmarks #1593
Commit: e5f0b51

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	71.36ms - 72.95ms	-	unsure 🔍 -1% - +2% -0.71ms - +1.51ms	unsure 🔍 -2% - +2% -1.09ms - +1.26ms
tip-of-tree tip-of-tree	70.98ms - 72.54ms	unsure 🔍 -2% - +1% -1.51ms - +0.71ms	-	unsure 🔍 -2% - +1% -1.48ms - +0.85ms
previous-release previous-release	71.20ms - 72.94ms	unsure 🔍 -2% - +2% -1.26ms - +1.09ms	unsure 🔍 -1% - +2% -0.85ms - +1.48ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	828.38ms - 841.72ms	-	unsure 🔍 -2% - +1% -14.65ms - +5.33ms	faster ✔ 0% - 2% 1.13ms - 20.17ms
tip-of-tree tip-of-tree	832.28ms - 847.14ms	unsure 🔍 -1% - +2% -5.33ms - +14.65ms	-	unsure 🔍 -2% - +0% -16.06ms - +4.08ms
previous-release previous-release	838.91ms - 852.49ms	slower ❌ 0% - 2% 1.13ms - 20.17ms	unsure 🔍 -0% - +2% -4.08ms - +16.06ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	907.82ms - 924.32ms	-	unsure 🔍 -1% - +2% -6.85ms - +15.49ms	unsure 🔍 -1% - +1% -8.76ms - +12.49ms
tip-of-tree tip-of-tree	904.21ms - 919.29ms	unsure 🔍 -2% - +1% -15.49ms - +6.85ms	-	unsure 🔍 -1% - +1% -12.54ms - +7.63ms
previous-release previous-release	907.51ms - 920.90ms	unsure 🔍 -1% - +1% -12.49ms - +8.76ms	unsure 🔍 -1% - +1% -7.63ms - +12.54ms	-

_{tachometer-reporter-action v2 for Benchmarks}

kevinpschaaf

LGTM

kevinpschaaf · 2021-07-13T21:30:17Z

packages/reactive-element/src/polyfill-support.ts

@@ -145,12 +145,11 @@ interface PatchableReactiveElement extends HTMLElement {
    this: PatchableReactiveElement,
    changedProperties: unknown
  ) {
-    const isFirstUpdate = !this.hasUpdated;
-    didUpdate.call(this, changedProperties);
    // Note, must do first update here so rendering has completed before
    // calling this and styles are correct by updated/firstUpdated.


This comment is pretty ambiguous; what does "first update" refer to (and why is the comment valid both before and after this change)?

sorvell requested a review from kevinpschaaf July 13, 2021 20:57

sorvell requested a review from justinfagnani as a code owner July 13, 2021 20:57

google-cla bot added the cla: yes label Jul 13, 2021

Add changeset.

5d12bf9

kevinpschaaf approved these changes Jul 14, 2021

View reviewed changes

Fix test helper so it doesn't blow away shimmed adoptedStylesheets

5251e3b

sorvell merged commit ff0d155 into main Jul 14, 2021

sorvell deleted the polyfill-styling-fixes branch July 14, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct polyfill-support styling issues #2002

Correct polyfill-support styling issues #2002

sorvell commented Jul 13, 2021

changeset-bot bot commented Jul 13, 2021 •

edited

github-actions bot commented Jul 13, 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

kevinpschaaf left a comment

kevinpschaaf Jul 13, 2021

Correct polyfill-support styling issues #2002

Correct polyfill-support styling issues #2002

Conversation

sorvell commented Jul 13, 2021

changeset-bot bot commented Jul 13, 2021 • edited

🦋 Changeset detected

github-actions bot commented Jul 13, 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

kevinpschaaf left a comment

Choose a reason for hiding this comment

kevinpschaaf Jul 13, 2021

Choose a reason for hiding this comment

changeset-bot bot commented Jul 13, 2021 •

edited

github-actions bot commented Jul 13, 2021 •

edited