[lit-html] Static `html` should not add consumed value to `TemplateResult` by augustjk · Pull Request #3825 · lit/lit

augustjk · 2023-04-20T23:12:08Z

SSR of static html was erroring due to mismatch of partIndex as described in issue above because we were comparing that to the length of the values array in TemplateResult.

When a static value was the last value of a template, it ended up adding the static value object to the TemplateResult. This wasn't an issue if a non-static value was the last value.

i.e.

const tag = literal`p`;
const template = html`<${tag}>Hello, ${this.name}!</${tag}`;

resulted in

{
  strings: ['<p>Hello, ', '!</p>'],
  values: ['World', {_$litStatic$: 'p', r: Symbol()}],
}

when it should be

{
  strings: ['<p>Hello, ', '!</p>'],
  values: ['World'],
}

In client rendering this wasn't an issue as we just never accessed that value. In SSR, it was causing the error due to the unexpected extra length.

changeset-bot · 2023-04-20T23:12:11Z

🦋 Changeset detected

Latest commit: 1399080

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

This PR includes changesets to release 2 packages

Name	Type
lit-html	Patch
lit	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-04-20T23:12:32Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -6% - +2% (-1.34ms - +0.50ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 105.79ms - 114.57ms
lit-html-kitchen-sink: unsure 🔍 -5% - +6% (-2.29ms - +2.51ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -9% - +4% (-1.22ms - +0.58ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -5% - +0% (-4.03ms - +0.26ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -5% - +2% (-4.17ms - +1.93ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 1098.69ms - 1117.84ms
lit-html-kitchen-sink: unsure 🔍 -7% - +3% (-7.65ms - +3.01ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -2% - +3% (-7.87ms - +10.06ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +3% (-3.12ms - +4.68ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +0% (-18.87ms - +5.10ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 1083.76ms - 1105.89ms
reactive-element-list: unsure 🔍 -2% - +1% (-20.85ms - +9.17ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 112.0.5615.121
Sample size: 50
Built by: Benchmarks #4667
Commit: b3b5ece

render

Version	Avg time	vs
	105.79ms - 114.57ms	-

update

Version	Avg time	vs
	1098.69ms - 1117.84ms	-

update-reflect

Version	Avg time	vs
	1083.76ms - 1105.89ms	-

lit-html-kitchen-sink

Browser: chrome-headless 112.0.5615.121
Sample size: 30
Built by: Benchmarks #4667
Commit: b3b5ece

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	42.43ms - 46.40ms	-	unsure 🔍 -5% - +6% -2.29ms - +2.51ms	unsure 🔍 -4% - +6% -1.84ms - +2.85ms
tip-of-tree tip-of-tree	42.96ms - 45.66ms	unsure 🔍 -6% - +5% -2.51ms - +2.29ms	-	unsure 🔍 -3% - +5% -1.45ms - +2.23ms
previous-release previous-release	42.66ms - 45.16ms	unsure 🔍 -6% - +4% -2.85ms - +1.84ms	unsure 🔍 -5% - +3% -2.23ms - +1.45ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	103.71ms - 110.57ms	-	unsure 🔍 -7% - +3% -7.65ms - +3.01ms	unsure 🔍 -7% - +2% -8.33ms - +2.17ms
tip-of-tree tip-of-tree	105.39ms - 113.54ms	unsure 🔍 -3% - +7% -3.01ms - +7.65ms	-	unsure 🔍 -6% - +4% -6.45ms - +4.93ms
previous-release previous-release	106.26ms - 114.19ms	unsure 🔍 -2% - +8% -2.17ms - +8.33ms	unsure 🔍 -5% - +6% -4.93ms - +6.45ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	20.01ms - 21.04ms	-	unsure 🔍 -6% - +2% -1.34ms - +0.50ms	unsure 🔍 -3% - +3% -0.58ms - +0.69ms
tip-of-tree tip-of-tree	20.19ms - 21.71ms	unsure 🔍 -2% - +7% -0.50ms - +1.34ms	-	unsure 🔍 -2% - +6% -0.37ms - +1.32ms
previous-release previous-release	20.10ms - 20.84ms	unsure 🔍 -3% - +3% -0.69ms - +0.58ms	unsure 🔍 -6% - +2% -1.32ms - +0.37ms	-

lit-html-repeat

Browser: chrome-headless 112.0.5615.121
Sample size: 30
Built by: Benchmarks #4667
Commit: b3b5ece

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	13.17ms - 14.50ms	-	unsure 🔍 -9% - +4% -1.22ms - +0.58ms	unsure 🔍 -8% - +5% -1.17ms - +0.73ms
tip-of-tree tip-of-tree	13.55ms - 14.76ms	unsure 🔍 -4% - +9% -0.58ms - +1.22ms	-	unsure 🔍 -6% - +7% -0.81ms - +1.01ms
previous-release previous-release	13.38ms - 14.73ms	unsure 🔍 -5% - +8% -0.73ms - +1.17ms	unsure 🔍 -7% - +6% -1.01ms - +0.81ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	336.25ms - 348.71ms	-	unsure 🔍 -2% - +3% -7.87ms - +10.06ms	unsure 🔍 -2% - +3% -7.02ms - +10.86ms
tip-of-tree tip-of-tree	334.93ms - 347.84ms	unsure 🔍 -3% - +2% -10.06ms - +7.87ms	-	unsure 🔍 -2% - +3% -8.28ms - +9.92ms
previous-release previous-release	334.15ms - 346.98ms	unsure 🔍 -3% - +2% -10.86ms - +7.02ms	unsure 🔍 -3% - +2% -9.92ms - +8.28ms	-

lit-html-template-heavy

Browser: chrome-headless 112.0.5615.121
Sample size: 50
Built by: Benchmarks #4667
Commit: b3b5ece

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	71.67ms - 74.99ms	-	unsure 🔍 -5% - +0% -4.03ms - +0.26ms	unsure 🔍 -4% - +3% -2.69ms - +1.89ms
tip-of-tree tip-of-tree	73.86ms - 76.57ms	unsure 🔍 -0% - +6% -0.26ms - +4.03ms	-	unsure 🔍 -1% - +5% -0.59ms - +3.57ms
previous-release previous-release	72.15ms - 75.30ms	unsure 🔍 -3% - +4% -1.89ms - +2.69ms	unsure 🔍 -5% - +1% -3.57ms - +0.59ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	153.93ms - 159.50ms	-	unsure 🔍 -2% - +3% -3.12ms - +4.68ms	unsure 🔍 -3% - +2% -4.57ms - +3.25ms
tip-of-tree tip-of-tree	153.21ms - 158.67ms	unsure 🔍 -3% - +2% -4.68ms - +3.12ms	-	unsure 🔍 -3% - +2% -5.30ms - +2.44ms
previous-release previous-release	154.63ms - 160.11ms	unsure 🔍 -2% - +3% -3.25ms - +4.57ms	unsure 🔍 -2% - +3% -2.44ms - +5.30ms	-

reactive-element-list

Browser: chrome-headless 112.0.5615.121
Sample size: 50
Built by: Benchmarks #4667
Commit: b3b5ece

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	73.52ms - 77.57ms	-	unsure 🔍 -5% - +2% -4.17ms - +1.93ms	unsure 🔍 -7% - +1% -5.22ms - +0.87ms
tip-of-tree tip-of-tree	74.39ms - 78.94ms	unsure 🔍 -3% - +6% -1.93ms - +4.17ms	-	unsure 🔍 -5% - +3% -4.28ms - +2.17ms
previous-release previous-release	75.44ms - 80.00ms	unsure 🔍 -1% - +7% -0.87ms - +5.22ms	unsure 🔍 -3% - +6% -2.17ms - +4.28ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1127.85ms - 1145.75ms	-	unsure 🔍 -2% - +0% -18.87ms - +5.10ms	unsure 🔍 -2% - +1% -17.47ms - +7.20ms
tip-of-tree tip-of-tree	1135.72ms - 1151.65ms	unsure 🔍 -0% - +2% -5.10ms - +18.87ms	-	unsure 🔍 -1% - +1% -9.89ms - +13.38ms
previous-release previous-release	1133.45ms - 1150.42ms	unsure 🔍 -1% - +2% -7.20ms - +17.47ms	unsure 🔍 -1% - +1% -13.38ms - +9.89ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1142.28ms - 1165.77ms	-	unsure 🔍 -2% - +1% -20.85ms - +9.17ms	unsure 🔍 -1% - +1% -16.60ms - +14.60ms
tip-of-tree tip-of-tree	1150.51ms - 1169.21ms	unsure 🔍 -1% - +2% -9.17ms - +20.85ms	-	unsure 🔍 -1% - +2% -9.04ms - +18.72ms
previous-release previous-release	1144.76ms - 1165.28ms	unsure 🔍 -1% - +1% -14.60ms - +16.60ms	unsure 🔍 -2% - +1% -18.72ms - +9.04ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani

Thanks for the quick turnaround!

justinfagnani · 2023-04-20T23:34:44Z

packages/lit-html/src/static.ts

-      dynamicValues.push(dynamicValue);
+      // If the last value is static, we don't need to push it.
+      if (i !== l) {
+        dynamicValues.push(dynamicValue);


Looking back at the original code, I don't love that dynamicValue apparently can hold a static value. Nice fix for now, but makes me want to clean this up later.

augustjk · 2023-04-21T01:01:24Z

Failing tests here due to #3827
This PR #3828 is a band-aid for this issue if we want to unblock our tests for now.

augustjk added 2 commits April 20, 2023 15:54

Don't add consumed static value to template result

3880d63

Add changeset

18cfbea

augustjk requested a review from kevinpschaaf April 20, 2023 23:12

augustjk requested a review from justinfagnani as a code owner April 20, 2023 23:12

Add SSR test for static html

1399080

justinfagnani approved these changes Apr 20, 2023

View reviewed changes

augustjk merged commit 343187b into main Apr 21, 2023

augustjk deleted the fix-static-html-values branch April 21, 2023 04:56

lit-robot mentioned this pull request Apr 25, 2023

Version Packages #3841

Merged

augustjk mentioned this pull request Nov 8, 2023

added failing test case for #2246 #2247

Closed

Conversation

augustjk commented Apr 20, 2023

Uh oh!

changeset-bot bot commented Apr 20, 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 Apr 20, 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!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

justinfagnani Apr 20, 2023

Choose a reason for hiding this comment

Uh oh!

augustjk commented Apr 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot bot commented Apr 20, 2023 •

edited

Loading

github-actions bot commented Apr 20, 2023 •

edited

Loading