[lit-html][labs/ssr-client] Fix compiled template support by AndrewJakubowicz · Pull Request #3993 · lit/lit

AndrewJakubowicz · 2023-07-06T04:53:25Z

Change is an independent part of #3984

Summary

Fix isTemplateResult return type to include CompiledTemplateResults, and then fix locations in lit that didn't handle CompiledTemplates. Primarily the cache directive.

Changes

Fixes the return type of isTemplateResult to return the correct type (as per current implementation): a TemplateResult or a CompiledTemplateResult.
Fix cache directive to work with both uncompiled and compiled templates using a helper to access the template strings array.
Made it explicit that hydration and experimental-hydration do not work with compiled templates by making the check more strict. Maybe could add a dev mode error?
Added compiled template cache directive unit test, and isTemplateResult type tests.

Considerations

I marked this as a minor change, however it is a breaking change in terms of TypeScript types. The return type of isTemplateResult without a second argument now has an additional consideration and could break users of isTemplateResult. However the type now more truthfully conveys what isTemplateResult is asserting.

Alternative considered:

Make isTemplateResult stricter such that the type is only a TemplateResult. A breaking change (although people should realistically not be depending on CompiledTemplateResults yet)
Add the following helpers: isCompiledTemplateResult to check for CompiledTemplates, and getTemplateStringsArray to generically retrieve the template strings array from either a compiled or uncompiled template result.

Test plan

Unit tests and type only tests added.

This commit fixes the type of `isTemplateResult` to return the correct type as the current implementation: a `TemplateResult` and a `CompiledTemplateResult`. Then I fixed the cache directive to work with both template types, and hydration to only work with uncompiled templates. Added a unit test for the compiled template case with the cache directive. An alternative I considered: - Make `isTemplateResult` stricter such that the type is only a `TemplateResult`. That is technically a breaking change (although people should not be depending on `CompiledTemplateResult`s) - And add the following helpers: `isCompiledTemplateResult` and `getTemplateStringsArray`.

changeset-bot · 2023-07-06T04:53:27Z

🦋 Changeset detected

Latest commit: 089e81a

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

This PR includes changesets to release 3 packages

Name	Type
lit-html	Minor
lit	Minor
@lit-labs/ssr-client	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-07-06T04:53:51Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -11% - +11% (-2.68ms - +2.56ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 72.25ms - 75.00ms
lit-html-kitchen-sink: unsure 🔍 -7% - +8% (-2.10ms - +2.55ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -7% - +23% (-0.70ms - +2.40ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-1.11ms - +1.34ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +3% (-0.71ms - +1.25ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 877.79ms - 887.57ms
lit-html-kitchen-sink: unsure 🔍 -8% - +3% (-6.79ms - +2.44ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -1% - +4% (-4.01ms - +11.03ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +3% (-0.89ms - +4.17ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +1% (-8.87ms - +6.04ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 829.06ms - 839.11ms
reactive-element-list: unsure 🔍 -1% - +0% (-8.25ms - +3.73ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 114.0.5735.198
Sample size: 50
Built by: Benchmarks #4998
Commit: 9a7f168

render

Version	Avg time	vs
	72.25ms - 75.00ms	-

update

Version	Avg time	vs
	877.79ms - 887.57ms	-

update-reflect

Version	Avg time	vs
	829.06ms - 839.11ms	-

lit-html-kitchen-sink

Browser: chrome-headless 114.0.5735.198
Sample size: 30
Built by: Benchmarks #4998
Commit: 9a7f168

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	29.70ms - 32.87ms	-	unsure 🔍 -7% - +8% -2.10ms - +2.55ms	unsure 🔍 -9% - +5% -2.90ms - +1.75ms
tip-of-tree tip-of-tree	29.36ms - 32.76ms	unsure 🔍 -8% - +7% -2.55ms - +2.10ms	-	unsure 🔍 -10% - +5% -3.21ms - +1.60ms
previous-release previous-release	30.16ms - 33.57ms	unsure 🔍 -6% - +9% -1.75ms - +2.90ms	unsure 🔍 -5% - +10% -1.60ms - +3.21ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	83.95ms - 90.20ms	-	unsure 🔍 -8% - +3% -6.79ms - +2.44ms	unsure 🔍 -5% - +5% -4.50ms - +4.10ms
tip-of-tree tip-of-tree	85.85ms - 92.64ms	unsure 🔍 -3% - +8% -2.44ms - +6.79ms	-	unsure 🔍 -3% - +7% -2.52ms - +6.47ms
previous-release previous-release	84.32ms - 90.22ms	unsure 🔍 -5% - +5% -4.10ms - +4.50ms	unsure 🔍 -7% - +3% -6.47ms - +2.52ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	22.47ms - 25.83ms	-	unsure 🔍 -11% - +11% -2.68ms - +2.56ms	slower ❌ 0% - 22% 0.20ms - 4.64ms
tip-of-tree tip-of-tree	22.20ms - 26.23ms	unsure 🔍 -11% - +11% -2.56ms - +2.68ms	-	unsure 🔍 -0% - +23% +0.00ms - +4.97ms
previous-release previous-release	20.28ms - 23.18ms	faster ✔ 1% - 19% 0.20ms - 4.64ms	faster ✔ 1% - 20% 0.00ms - 4.97ms	-

lit-html-repeat

Browser: chrome-headless 114.0.5735.198
Sample size: 30
Built by: Benchmarks #4998
Commit: 9a7f168

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	9.80ms - 12.88ms	-	unsure 🔍 -7% - +23% -0.70ms - +2.40ms	unsure 🔍 -5% - +25% -0.53ms - +2.58ms
tip-of-tree tip-of-tree	10.28ms - 10.70ms	unsure 🔍 -20% - +5% -2.40ms - +0.70ms	-	unsure 🔍 -2% - +5% -0.16ms - +0.50ms
previous-release previous-release	10.06ms - 10.57ms	unsure 🔍 -22% - +3% -2.58ms - +0.53ms	unsure 🔍 -5% - +1% -0.50ms - +0.16ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	307.50ms - 319.09ms	-	unsure 🔍 -1% - +4% -4.01ms - +11.03ms	unsure 🔍 -1% - +5% -3.88ms - +14.54ms
tip-of-tree tip-of-tree	305.00ms - 314.58ms	unsure 🔍 -4% - +1% -11.03ms - +4.01ms	-	unsure 🔍 -2% - +3% -6.80ms - +10.44ms
previous-release previous-release	300.80ms - 315.13ms	unsure 🔍 -5% - +1% -14.54ms - +3.88ms	unsure 🔍 -3% - +2% -10.44ms - +6.80ms	-

lit-html-template-heavy

Browser: chrome-headless 114.0.5735.198
Sample size: 50
Built by: Benchmarks #4998
Commit: 9a7f168

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.22ms - 54.65ms	-	unsure 🔍 -2% - +2% -1.11ms - +1.34ms	unsure 🔍 -4% - +0% -2.32ms - +0.10ms
tip-of-tree tip-of-tree	52.82ms - 54.81ms	unsure 🔍 -2% - +2% -1.34ms - +1.11ms	-	unsure 🔍 -5% - +0% -2.62ms - +0.16ms
previous-release previous-release	54.07ms - 56.01ms	unsure 🔍 -0% - +4% -0.10ms - +2.32ms	unsure 🔍 -0% - +5% -0.16ms - +2.62ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	128.06ms - 132.08ms	-	unsure 🔍 -1% - +3% -0.89ms - +4.17ms	unsure 🔍 -2% - +2% -2.10ms - +2.86ms
tip-of-tree tip-of-tree	126.89ms - 129.96ms	unsure 🔍 -3% - +1% -4.17ms - +0.89ms	-	unsure 🔍 -3% - +1% -3.38ms - +0.85ms
previous-release previous-release	128.23ms - 131.15ms	unsure 🔍 -2% - +2% -2.86ms - +2.10ms	unsure 🔍 -1% - +3% -0.85ms - +3.38ms	-

reactive-element-list

Browser: chrome-headless 114.0.5735.198
Sample size: 50
Built by: Benchmarks #4998
Commit: 9a7f168

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	47.05ms - 48.55ms	-	unsure 🔍 -1% - +3% -0.71ms - +1.25ms	unsure 🔍 -1% - +3% -0.62ms - +1.40ms
tip-of-tree tip-of-tree	46.90ms - 48.16ms	unsure 🔍 -3% - +1% -1.25ms - +0.71ms	-	unsure 🔍 -2% - +2% -0.80ms - +1.04ms
previous-release previous-release	46.74ms - 48.08ms	unsure 🔍 -3% - +1% -1.40ms - +0.62ms	unsure 🔍 -2% - +2% -1.04ms - +0.80ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	928.85ms - 938.68ms	-	unsure 🔍 -1% - +1% -8.87ms - +6.04ms	unsure 🔍 -1% - +1% -7.81ms - +5.90ms
tip-of-tree tip-of-tree	929.57ms - 940.78ms	unsure 🔍 -1% - +1% -6.04ms - +8.87ms	-	unsure 🔍 -1% - +1% -6.91ms - +7.83ms
previous-release previous-release	929.94ms - 939.50ms	unsure 🔍 -1% - +1% -5.90ms - +7.81ms	unsure 🔍 -1% - +1% -7.83ms - +6.91ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	894.05ms - 901.23ms	-	unsure 🔍 -1% - +0% -8.25ms - +3.73ms	unsure 🔍 -0% - +1% -4.02ms - +6.61ms
tip-of-tree tip-of-tree	895.10ms - 904.70ms	unsure 🔍 -0% - +1% -3.73ms - +8.25ms	-	unsure 🔍 -0% - +1% -2.64ms - +9.75ms
previous-release previous-release	892.42ms - 900.27ms	unsure 🔍 -1% - +0% -6.61ms - +4.02ms	unsure 🔍 -1% - +0% -9.75ms - +2.64ms	-

_{tachometer-reporter-action v2 for Benchmarks}

rictic

Looks good!

packages/lit-html/src/directive-helpers.ts

augustjk

Feel free to push back on any requested changes.

packages/lit-html/src/directive-helpers.ts

.changeset/chilled-drinks-travel.md

packages/labs/ssr-client/src/lib/hydrate-lit-html.ts

AndrewJakubowicz · 2023-07-07T16:03:12Z

I added two more small cleanup commits. 1. Use the isCompiledTemplate helper in the cache directive fix to simplify some logic. (No behavior change). 2. Add type only tests to verify the excellent improvement in returning the exact (HTML or SVG) TemplateResult type from isTemplateResult.

PTAL

packages/lit-html/src/directive-helpers.ts

packages/lit-html/src/directives/cache.ts

AndrewJakubowicz added 2 commits July 6, 2023 14:39

Apply some fixes after discussion with Justin and Peter

74d045c

add changeset

d994aa1

AndrewJakubowicz marked this pull request as ready for review July 6, 2023 21:46

AndrewJakubowicz requested a review from kevinpschaaf as a code owner July 6, 2023 21:46

AndrewJakubowicz requested review from augustjk, justinfagnani and rictic July 6, 2023 21:48

ssr-client is a patch change - because only error text has changed

4e592de

rictic requested changes Jul 6, 2023

View reviewed changes

packages/lit-html/src/directive-helpers.ts Show resolved Hide resolved

augustjk requested changes Jul 6, 2023

View reviewed changes

AndrewJakubowicz added 3 commits July 6, 2023 16:44

Code review feedback

1f244fc

split changeset

e3a926c

fix typo

680ec7e

rictic approved these changes Jul 7, 2023

View reviewed changes

augustjk approved these changes Jul 7, 2023

View reviewed changes

AndrewJakubowicz added 2 commits July 7, 2023 08:54

Simplify getStringsFromTemplate to use isCompiled helper

fb5f3d5

add more specific type only tests for isTemplateResult

8b9c2e0

justinfagnani reviewed Jul 7, 2023

View reviewed changes

AndrewJakubowicz added 2 commits July 7, 2023 10:02

Justin's code review feedback

5bc14b6

Reduce number of function calls in cache directive

089e81a

augustjk approved these changes Jul 7, 2023

View reviewed changes

AndrewJakubowicz force-pushed the fix-cache-directive branch from ffefb27 to 089e81a Compare July 7, 2023 17:50

AndrewJakubowicz requested a review from justinfagnani July 7, 2023 18:45

justinfagnani approved these changes Jul 10, 2023

View reviewed changes

AndrewJakubowicz merged commit e2c5056 into main Jul 10, 2023

AndrewJakubowicz deleted the fix-cache-directive branch July 10, 2023 20:22

lit-robot mentioned this pull request Aug 2, 2023

Version Packages #4066

Merged

Conversation

AndrewJakubowicz commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Considerations

Alternative considered:

Test plan

Uh oh!

changeset-bot bot commented Jul 6, 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 Jul 6, 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!

rictic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewJakubowicz commented Jul 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndrewJakubowicz commented Jul 6, 2023 •

edited

Loading

changeset-bot bot commented Jul 6, 2023 •

edited

Loading

github-actions bot commented Jul 6, 2023 •

edited

Loading