[labs/analyzer] Add template utils to analyzer and use them in the compiler #4261

justinfagnani · 2023-10-04T18:45:36Z

Builds on #4260 (for use in tests)

This moves the isLitTaggedTemplateExpression template utility from the compiler to the analyzer. The analyzer will need these same utilities to detect lit-html template expressions for template analysis.

changeset-bot · 2023-10-04T18:45:39Z

🦋 Changeset detected

Latest commit: 6bf89aa

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

This PR includes changesets to release 8 packages

Name	Type
@lit-labs/analyzer	Minor
@lit-labs/cli	Patch
@lit-labs/compiler	Patch
@lit-labs/gen-manifest	Patch
@lit-labs/gen-utils	Patch
@lit-labs/gen-wrapper-angular	Patch
@lit-labs/gen-wrapper-react	Patch
@lit-labs/gen-wrapper-vue	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-10-04T18:45:53Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +8% (-0.19ms - +0.84ms)
^{this-change vs tip-of-tree}

render

this-change: 47.16ms - 49.63ms
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +3% (-0.72ms - +0.63ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-1.19ms - +0.38ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +2% (-0.82ms - +0.52ms)
^{this-change vs tip-of-tree}

update

this-change: 539.79ms - 551.22ms
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +6% (-2.82ms - +2.32ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -6% - +1% (-4.20ms - +0.56ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-12.47ms - +9.33ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 537.59ms - 548.88ms
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-11.97ms - +9.34ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6089
Commit: 6bf89aa

render

Version	Avg time	vs
	47.16ms - 49.63ms	-

update

Version	Avg time	vs
	539.79ms - 551.22ms	-

update-reflect

Version	Avg time	vs
	537.59ms - 548.88ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 121.0.6167.85
Sample size: 30
Built by: Benchmarks #6089
Commit: 6bf89aa

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.19ms - 19.09ms	-	unsure 🔍 -4% - +3% -0.72ms - +0.63ms	unsure 🔍 -3% - +4% -0.53ms - +0.79ms
tip-of-tree tip-of-tree	18.18ms - 19.19ms	unsure 🔍 -3% - +4% -0.63ms - +0.72ms	-	unsure 🔍 -3% - +5% -0.53ms - +0.88ms
previous-release previous-release	18.02ms - 19.00ms	unsure 🔍 -4% - +3% -0.79ms - +0.53ms	unsure 🔍 -5% - +3% -0.88ms - +0.53ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	38.28ms - 42.01ms	-	unsure 🔍 -7% - +6% -2.82ms - +2.32ms	unsure 🔍 -11% - +1% -4.59ms - +0.53ms
tip-of-tree tip-of-tree	38.63ms - 42.16ms	unsure 🔍 -6% - +7% -2.32ms - +2.82ms	-	unsure 🔍 -10% - +2% -4.27ms - +0.71ms
previous-release previous-release	40.42ms - 43.93ms	unsure 🔍 -1% - +12% -0.53ms - +4.59ms	unsure 🔍 -2% - +11% -0.71ms - +4.27ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.76ms - 11.47ms	-	unsure 🔍 -2% - +8% -0.19ms - +0.84ms	unsure 🔍 -7% - +1% -0.77ms - +0.17ms
tip-of-tree tip-of-tree	10.42ms - 11.16ms	unsure 🔍 -7% - +2% -0.84ms - +0.19ms	-	faster ✔ 1% - 10% 0.14ms - 1.10ms
previous-release previous-release	11.11ms - 11.71ms	unsure 🔍 -2% - +7% -0.17ms - +0.77ms	slower ❌ 1% - 10% 0.14ms - 1.10ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6089
Commit: 6bf89aa

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.28ms - 34.29ms	-	unsure 🔍 -3% - +1% -1.19ms - +0.38ms	unsure 🔍 -3% - +1% -1.13ms - +0.35ms
tip-of-tree tip-of-tree	33.59ms - 34.78ms	unsure 🔍 -1% - +4% -0.38ms - +1.19ms	-	unsure 🔍 -2% - +2% -0.79ms - +0.82ms
previous-release previous-release	33.63ms - 34.71ms	unsure 🔍 -1% - +3% -0.35ms - +1.13ms	unsure 🔍 -2% - +2% -0.82ms - +0.79ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	71.22ms - 74.20ms	-	unsure 🔍 -6% - +1% -4.20ms - +0.56ms	unsure 🔍 -4% - +2% -2.91ms - +1.64ms
tip-of-tree tip-of-tree	72.68ms - 76.39ms	unsure 🔍 -1% - +6% -0.56ms - +4.20ms	-	unsure 🔍 -2% - +5% -1.34ms - +3.72ms
previous-release previous-release	71.63ms - 75.06ms	unsure 🔍 -2% - +4% -1.64ms - +2.91ms	unsure 🔍 -5% - +2% -3.72ms - +1.34ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 121.0.6167.85
Sample size: 50
Built by: Benchmarks #6089
Commit: 6bf89aa

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.07ms - 31.94ms	-	unsure 🔍 -3% - +2% -0.82ms - +0.52ms	unsure 🔍 -2% - +2% -0.73ms - +0.63ms
tip-of-tree tip-of-tree	31.15ms - 32.16ms	unsure 🔍 -2% - +3% -0.52ms - +0.82ms	-	unsure 🔍 -2% - +3% -0.63ms - +0.83ms
previous-release previous-release	31.03ms - 32.09ms	unsure 🔍 -2% - +2% -0.63ms - +0.73ms	unsure 🔍 -3% - +2% -0.83ms - +0.63ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	511.69ms - 526.19ms	-	unsure 🔍 -2% - +2% -12.47ms - +9.33ms	unsure 🔍 -2% - +2% -11.80ms - +10.37ms
tip-of-tree tip-of-tree	512.37ms - 528.64ms	unsure 🔍 -2% - +2% -9.33ms - +12.47ms	-	unsure 🔍 -2% - +2% -10.83ms - +12.54ms
previous-release previous-release	511.26ms - 528.04ms	unsure 🔍 -2% - +2% -10.37ms - +11.80ms	unsure 🔍 -2% - +2% -12.54ms - +10.83ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	515.93ms - 530.39ms	-	unsure 🔍 -2% - +2% -11.97ms - +9.34ms	unsure 🔍 -2% - +2% -13.04ms - +8.35ms
tip-of-tree tip-of-tree	516.66ms - 532.30ms	unsure 🔍 -2% - +2% -9.34ms - +11.97ms	-	unsure 🔍 -2% - +2% -12.13ms - +10.07ms
previous-release previous-release	517.63ms - 533.38ms	unsure 🔍 -2% - +2% -8.35ms - +13.04ms	unsure 🔍 -2% - +2% -10.07ms - +12.13ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2023-10-04T18:48:00Z

The size of lit-html.js and lit-core.min.js are as expected.

packages/labs/analyzer/src/lib/javascript/classes.ts

rictic · 2023-10-04T23:47:26Z

packages/labs/analyzer/src/lib/javascript/classes.ts

        ) {
-          const name = node.expression.left.name.getText();
+          const name = statement.expression.left.name.getText();


here and elsewhere: getText() is almost always a mistake to call outside of debug statements, because it exists on every node but doesn't have a reasonable value for a lot of them (and when called on artificial nodes that weren't in the original source code it may throw an error)

This is already used in a bunch of places. I can file an issue to change them.

Yeah, we should honestly add a lint rule to catch and prevent calls to getText(). We did that on the internal version of grimlock and it prevented a bunch of issues.

Andrew actually ran into an issue today where .getText() did the wrong thing (when called on a SourceFile it doesn't include all the content of the file (!), it misses leading comments. SourceFile has a .text property with the full text of the file)

packages/labs/analyzer/src/lib/model.ts

rictic

LGTM, but for reals, every .getText() is a bug waiting to happen

packages/labs/analyzer/src/test/server/lit-html/template_test.ts

justinfagnani requested review from rictic and AndrewJakubowicz October 4, 2023 18:45

justinfagnani requested a review from kevinpschaaf as a code owner October 4, 2023 18:45

justinfagnani mentioned this pull request Oct 4, 2023

fix (labs/analyzer): use analyzer typescript instead of import #4252

Merged

rictic reviewed Oct 4, 2023

View reviewed changes

justinfagnani mentioned this pull request Oct 6, 2023

[labs/analyzer] Add template parser to analyzer #4267

Open

Base automatically changed from 3.0 to main October 9, 2023 16:54

justinfagnani changed the base branch from main to analyzer-nodes October 27, 2023 18:29

Base automatically changed from analyzer-nodes to main October 27, 2023 22:33

Add template utils to analyzer and use them in the compiler

b4c6ae2

justinfagnani force-pushed the analyzer-templates branch from 48032bd to b4c6ae2 Compare February 1, 2024 02:00

justinfagnani requested review from rictic and 43081j February 1, 2024 02:04

rictic approved these changes Feb 1, 2024

View reviewed changes

packages/labs/analyzer/src/test/server/lit-html/template_test.ts Outdated Show resolved Hide resolved

packages/labs/analyzer/src/test/server/lit-html/template_test.ts Outdated Show resolved Hide resolved

43081j approved these changes Feb 1, 2024

View reviewed changes

Address feedback

6bf89aa

justinfagnani merged commit 1b17a36 into main Feb 1, 2024
9 checks passed

justinfagnani deleted the analyzer-templates branch February 1, 2024 17:34

lit-robot mentioned this pull request Mar 11, 2024

Version Packages #4579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[labs/analyzer] Add template utils to analyzer and use them in the compiler #4261

[labs/analyzer] Add template utils to analyzer and use them in the compiler #4261

justinfagnani commented Oct 4, 2023

changeset-bot bot commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Oct 4, 2023 •

edited

Loading

rictic Oct 4, 2023

justinfagnani Oct 27, 2023

rictic Feb 1, 2024

AndrewJakubowicz Feb 1, 2024

rictic left a comment

[labs/analyzer] Add template utils to analyzer and use them in the compiler #4261

[labs/analyzer] Add template utils to analyzer and use them in the compiler #4261

Conversation

justinfagnani commented Oct 4, 2023

changeset-bot bot commented Oct 4, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Oct 4, 2023 • edited Loading

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

github-actions bot commented Oct 4, 2023 • edited Loading

rictic Oct 4, 2023

Choose a reason for hiding this comment

justinfagnani Oct 27, 2023

Choose a reason for hiding this comment

rictic Feb 1, 2024

Choose a reason for hiding this comment

AndrewJakubowicz Feb 1, 2024

Choose a reason for hiding this comment

rictic left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading

github-actions bot commented Oct 4, 2023 •

edited

Loading