[lit-html] Prevent `styleMap` from writing to the 'style' attribute. by bicknellr · Pull Request #1819 · lit/lit

bicknellr · 2021-04-28T00:30:18Z

Internally, styleMap can't update the style attribute dynamically because it isn't allowed by the sanitizer. However, this directive already handles updates using the style property, so this change falls through to use that in all cases.

github-actions · 2021-04-28T00:37:17Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -8% - +9% (-4.60ms - +4.92ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -3% - +1% (-3.55ms - +1.42ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -9% - +2% (-5.06ms - +0.99ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -6% - +2% (-0.89ms - +0.31ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -4% - +3% (-3.67ms - +2.64ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +1% (-2.65ms - +0.49ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -2% - +0% (-25.07ms - +5.05ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -5% - +5% (-8.47ms - +8.80ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -1% - +3% (-6.58ms - +12.32ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +4% (-3.91ms - +7.02ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +1% (-7.55ms - +14.29ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -1% - +1% (-11.52ms - +15.11ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +0% (-19.92ms - +4.39ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1796
Commit: 9d67052

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	120.69ms - 124.05ms	-	unsure 🔍 -3% - +1% -3.55ms - +1.42ms	faster ✔ 21% - 24% 32.60ms - 38.02ms
tip-of-tree tip-of-tree	121.61ms - 125.27ms	unsure 🔍 -1% - +3% -1.42ms - +3.55ms	-	faster ✔ 20% - 23% 31.44ms - 37.04ms
previous-release previous-release	155.55ms - 159.80ms	slower ❌ 26% - 31% 32.60ms - 38.02ms	slower ❌ 25% - 30% 31.44ms - 37.04ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1024.35ms - 1045.90ms	-	unsure 🔍 -2% - +0% -25.07ms - +5.05ms	faster ✔ 6% - 9% 71.86ms - 100.04ms
tip-of-tree tip-of-tree	1034.62ms - 1055.65ms	unsure 🔍 -0% - +2% -5.05ms - +25.07ms	-	faster ✔ 6% - 8% 62.05ms - 89.84ms
previous-release previous-release	1112.00ms - 1130.16ms	slower ❌ 7% - 10% 71.86ms - 100.04ms	slower ❌ 6% - 9% 62.05ms - 89.84ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1038.75ms - 1058.67ms	-	unsure 🔍 -1% - +1% -11.52ms - +15.11ms	faster ✔ 6% - 8% 61.66ms - 91.02ms
tip-of-tree tip-of-tree	1038.07ms - 1055.75ms	unsure 🔍 -1% - +1% -15.11ms - +11.52ms	-	faster ✔ 6% - 8% 64.19ms - 92.08ms
previous-release previous-release	1114.26ms - 1135.83ms	slower ❌ 6% - 9% 61.66ms - 91.02ms	slower ❌ 6% - 9% 64.19ms - 92.08ms	-

lit-html-kitchen-sink

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1796
Commit: 9d67052

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	52.13ms - 55.13ms	-	unsure 🔍 -9% - +2% -5.06ms - +0.99ms	faster ✔ 26% - 35% 19.29ms - 27.83ms
tip-of-tree tip-of-tree	53.04ms - 58.29ms	unsure 🔍 -2% - +9% -0.99ms - +5.06ms	-	faster ✔ 23% - 33% 16.74ms - 26.30ms
previous-release previous-release	73.19ms - 81.19ms	slower ❌ 35% - 52% 19.29ms - 27.83ms	slower ❌ 29% - 48% 16.74ms - 26.30ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	155.10ms - 166.88ms	-	unsure 🔍 -5% - +5% -8.47ms - +8.80ms	unsure 🔍 -7% - +5% -11.09ms - +8.31ms
tip-of-tree tip-of-tree	154.51ms - 167.14ms	unsure 🔍 -5% - +5% -8.80ms - +8.47ms	-	unsure 🔍 -7% - +5% -11.52ms - +8.41ms
previous-release previous-release	154.67ms - 170.09ms	unsure 🔍 -5% - +7% -8.31ms - +11.09ms	unsure 🔍 -5% - +7% -8.41ms - +11.52ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.34ms - 59.31ms	-	unsure 🔍 -8% - +9% -4.60ms - +4.92ms	slower ❌ 10% - 25% 4.89ms - 11.85ms
tip-of-tree tip-of-tree	52.45ms - 59.87ms	unsure 🔍 -9% - +8% -4.92ms - +4.60ms	-	slower ❌ 8% - 26% 4.09ms - 12.33ms
previous-release previous-release	46.16ms - 49.74ms	faster ✔ 9% - 20% 4.89ms - 11.85ms	faster ✔ 8% - 21% 4.09ms - 12.33ms	-

lit-html-repeat

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1796
Commit: 9d67052

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	14.79ms - 15.45ms	-	unsure 🔍 -6% - +2% -0.89ms - +0.31ms	faster ✔ 11% - 16% 1.95ms - 2.93ms
tip-of-tree tip-of-tree	14.90ms - 15.91ms	unsure 🔍 -2% - +6% -0.31ms - +0.89ms	-	faster ✔ 9% - 16% 1.53ms - 2.77ms
previous-release previous-release	17.19ms - 17.92ms	slower ❌ 13% - 20% 1.95ms - 2.93ms	slower ❌ 10% - 18% 1.53ms - 2.77ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	479.03ms - 492.42ms	-	unsure 🔍 -1% - +3% -6.58ms - +12.32ms	faster ✔ 27% - 30% 182.85ms - 203.64ms
tip-of-tree tip-of-tree	476.18ms - 489.52ms	unsure 🔍 -3% - +1% -12.32ms - +6.58ms	-	faster ✔ 28% - 30% 185.74ms - 206.49ms
previous-release previous-release	671.02ms - 686.92ms	slower ❌ 37% - 42% 182.85ms - 203.64ms	slower ❌ 38% - 43% 185.74ms - 206.49ms	-

lit-html-template-heavy

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1796
Commit: 9d67052

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	81.10ms - 85.42ms	-	unsure 🔍 -4% - +3% -3.67ms - +2.64ms	faster ✔ 14% - 19% 13.30ms - 19.69ms
tip-of-tree tip-of-tree	81.48ms - 86.08ms	unsure 🔍 -3% - +4% -2.64ms - +3.67ms	-	faster ✔ 13% - 19% 12.68ms - 19.27ms
previous-release previous-release	97.40ms - 102.12ms	slower ❌ 16% - 24% 13.30ms - 19.69ms	slower ❌ 15% - 23% 12.68ms - 19.27ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	172.68ms - 180.33ms	-	unsure 🔍 -2% - +4% -3.91ms - +7.02ms	faster ✔ 8% - 13% 16.08ms - 26.35ms
tip-of-tree tip-of-tree	171.04ms - 178.85ms	unsure 🔍 -4% - +2% -7.02ms - +3.91ms	-	faster ✔ 9% - 14% 17.57ms - 27.96ms
previous-release previous-release	194.29ms - 201.14ms	slower ❌ 9% - 15% 16.08ms - 26.35ms	slower ❌ 10% - 16% 17.57ms - 27.96ms	-

reactive-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1796
Commit: 9d67052

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	78.02ms - 79.93ms	-	unsure 🔍 -3% - +1% -2.65ms - +0.49ms	unsure 🔍 -2% - +2% -1.87ms - +1.37ms
tip-of-tree tip-of-tree	78.80ms - 81.30ms	unsure 🔍 -1% - +3% -0.49ms - +2.65ms	-	unsure 🔍 -1% - +3% -0.98ms - +2.63ms
previous-release previous-release	77.92ms - 80.53ms	unsure 🔍 -2% - +2% -1.37ms - +1.87ms	unsure 🔍 -3% - +1% -2.63ms - +0.98ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	996.43ms - 1012.29ms	-	unsure 🔍 -1% - +1% -7.55ms - +14.29ms	unsure 🔍 -1% - +1% -12.53ms - +10.01ms
tip-of-tree tip-of-tree	993.48ms - 1008.49ms	unsure 🔍 -1% - +1% -14.29ms - +7.55ms	-	unsure 🔍 -2% - +1% -15.61ms - +6.34ms
previous-release previous-release	997.61ms - 1013.63ms	unsure 🔍 -1% - +1% -10.01ms - +12.53ms	unsure 🔍 -1% - +2% -6.34ms - +15.61ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1060.48ms - 1077.34ms	-	unsure 🔍 -2% - +0% -19.92ms - +4.39ms	unsure 🔍 -2% - +0% -21.31ms - +4.49ms
tip-of-tree tip-of-tree	1067.92ms - 1085.43ms	unsure 🔍 -0% - +2% -4.39ms - +19.92ms	-	unsure 🔍 -1% - +1% -13.76ms - +12.48ms
previous-release previous-release	1067.55ms - 1087.08ms	unsure 🔍 -0% - +2% -4.49ms - +21.31ms	unsure 🔍 -1% - +1% -12.48ms - +13.76ms	-

_{tachometer-reporter-action v2 for Benchmarks}

kevinpschaaf

Need to fix TS errors in the build, plus this one nit.

kevinpschaaf · 2021-05-20T18:06:26Z

@@ -66,10 +66,6 @@ class StyleMapDirective extends Directive {

    if (this._previousStyleProperties === undefined) {


This entire if can go away and instead we start with L28 initializing _previousStyleProperties: Set<string> = new Set();

…-update

…sts to return a different type.

bicknellr · 2021-05-21T00:16:12Z

The tests are failing because of #1920.

justinfagnani · 2021-05-21T15:41:37Z

@bicknellr #1920 was about a directive returning noChange removing the attribute, while #1921 is about nothing removing the attribute. #1921 is intended behavior, but #1920 is not. I think #1921 should be closed and #1920 reopened.

justinfagnani · 2021-05-21T16:21:09Z

Actually, looking into this some... I'm not sure #1920 isn't WAI too. The question is what should render if we get noChange on first render? I'll take this to that issue, but here one workaround is to return undefined instead of noChange. That would emulate any change we might make for #1920 which would be to initialize with undefined instead of nothing.

…-update

changeset-bot · 2021-08-27T01:18:38Z

🦋 Changeset detected

Latest commit: a88cce2

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

…pdate`.

…-update

bicknellr · 2021-08-28T00:14:58Z

This still has legit failures that I need to look at.

Prevent styleMap from writing to the 'style' attribute.

cf360ec

google-cla Bot added the cla: yes label Apr 28, 2021

justinfagnani changed the title ~~Prevent styleMap from writing to the 'style' attribute.~~ [lit-html] Prevent styleMap from writing to the 'style' attribute. May 7, 2021

kevinpschaaf reviewed May 20, 2021

View reviewed changes

bicknellr added 3 commits May 20, 2021 13:15

Merge remote-tracking branch 'origin/main' into styleMap-no-attribute…

9cc16fc

…-update

Widen StyleMapDirective return type to allow an extension in the te…

646dbf1

…sts to return a different type.

Initialize _previousStyleProperties directly.

08c51b1

bicknellr mentioned this pull request May 24, 2021

Attribute content is cleared entirely if an expression in an AttributePart evaluates to noChange on first render #1920

Closed

Merge remote-tracking branch 'origin/main' into styleMap-no-attribute…

d8b8990

…-update

bicknellr added 4 commits August 26, 2021 18:40

Add a cast in the tests to avoid having to change the signature of `u…

a8cf720

…pdate`.

Merge remote-tracking branch 'origin/main' into styleMap-no-attribute…

b369efe

…-update

Add a changeset.

948cbe4

Explicitly set the return type to typeof noChange.

a88cce2

bicknellr force-pushed the styleMap-no-attribute-update branch from 09634ab to a88cce2 Compare August 27, 2021 21:53

		@@ -66,10 +66,6 @@ class StyleMapDirective extends Directive {

		if (this._previousStyleProperties === undefined) {

Uh oh!

Conversation

bicknellr commented Apr 28, 2021

Uh oh!

github-actions Bot commented Apr 28, 2021 • 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!

kevinpschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf May 20, 2021

Choose a reason for hiding this comment

Uh oh!

bicknellr commented May 21, 2021

Uh oh!

justinfagnani commented May 21, 2021

Uh oh!

justinfagnani commented May 21, 2021

Uh oh!

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

Uh oh!

🦋 Changeset detected

Uh oh!

bicknellr commented Aug 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Apr 28, 2021 •

edited

Loading

changeset-bot Bot commented Aug 27, 2021 •

edited

Loading