Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

justinfagnani
Copy link
Collaborator

Builds on #4261

Adds a parseLitTemplate() function to lib/lit-html/template.ts. This returns an extension of a parse5 DocumentFragment that contains parts and strings, and where some nodes like comments and attributes are annotated with Lit parts. The parts have references to their expressions to allow easy traversal into nested expressions and templates.

I didn't yet port over the similar code in SSR or the compiler. These might have some different requirements (like parallel traversal or re-writing comment markers). Other future extensions could be detecting common invalid binding locations for linting.

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2023

🦋 Changeset detected

Latest commit: 547fd74

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
Copy link
Contributor

github-actions bot commented Oct 6, 2023

📊 Tachometer Benchmark Results

Summary

nop-update

  • this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +8% (-0.44ms - +0.85ms)
    this-change vs tip-of-tree

render

  • this-change: 46.54ms - 48.58ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +2% (-0.92ms - +0.40ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +4% (-0.27ms - +1.25ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +5% (-0.58ms - +1.67ms)
    this-change vs tip-of-tree

update

  • this-change: 521.34ms - 531.44ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -6% - +8% (-2.31ms - +3.41ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +3% (-1.58ms - +2.01ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-10.62ms - +6.02ms)
    this-change vs tip-of-tree

update-reflect

  • this-change: 522.95ms - 530.59ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-3.74ms - +9.93ms)
    this-change vs tip-of-tree

Results

this-change

render

VersionAvg timevs
46.54ms - 48.58ms-

update

VersionAvg timevs
521.34ms - 531.44ms-

update-reflect

VersionAvg timevs
522.95ms - 530.59ms-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
18.51ms - 19.44ms-unsure 🔍
-5% - +2%
-0.92ms - +0.40ms
unsure 🔍
-2% - +5%
-0.35ms - +0.92ms
tip-of-tree
tip-of-tree
18.76ms - 19.71msunsure 🔍
-2% - +5%
-0.40ms - +0.92ms
-unsure 🔍
-1% - +6%
-0.10ms - +1.18ms
previous-release
previous-release
18.26ms - 19.13msunsure 🔍
-5% - +2%
-0.92ms - +0.35ms
unsure 🔍
-6% - +0%
-1.18ms - +0.10ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
40.08ms - 44.24ms-unsure 🔍
-6% - +8%
-2.31ms - +3.41ms
unsure 🔍
-4% - +11%
-1.44ms - +4.29ms
tip-of-tree
tip-of-tree
39.64ms - 43.58msunsure 🔍
-8% - +5%
-3.41ms - +2.31ms
-unsure 🔍
-5% - +9%
-1.91ms - +3.66ms
previous-release
previous-release
38.76ms - 42.71msunsure 🔍
-10% - +3%
-4.29ms - +1.44ms
unsure 🔍
-9% - +5%
-3.66ms - +1.91ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.02ms - 11.88ms-unsure 🔍
-4% - +8%
-0.44ms - +0.85ms
unsure 🔍
-2% - +9%
-0.25ms - +0.97ms
tip-of-tree
tip-of-tree
10.76ms - 11.72msunsure 🔍
-7% - +4%
-0.85ms - +0.44ms
-unsure 🔍
-5% - +7%
-0.50ms - +0.80ms
previous-release
previous-release
10.66ms - 11.53msunsure 🔍
-8% - +2%
-0.97ms - +0.25ms
unsure 🔍
-7% - +4%
-0.80ms - +0.50ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
34.00ms - 34.94ms-unsure 🔍
-1% - +4%
-0.27ms - +1.25ms
slower ❌
1% - 4%
0.26ms - 1.50ms
tip-of-tree
tip-of-tree
33.38ms - 34.57msunsure 🔍
-4% - +1%
-1.25ms - +0.27ms
-unsure 🔍
-1% - +3%
-0.33ms - +1.11ms
previous-release
previous-release
33.19ms - 33.99msfaster ✔
1% - 4%
0.26ms - 1.50ms
unsure 🔍
-3% - +1%
-1.11ms - +0.33ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
73.26ms - 75.62ms-unsure 🔍
-2% - +3%
-1.58ms - +2.01ms
unsure 🔍
-2% - +3%
-1.74ms - +2.02ms
tip-of-tree
tip-of-tree
72.88ms - 75.57msunsure 🔍
-3% - +2%
-2.01ms - +1.58ms
-unsure 🔍
-3% - +3%
-2.07ms - +1.91ms
previous-release
previous-release
72.84ms - 75.76msunsure 🔍
-3% - +2%
-2.02ms - +1.74ms
unsure 🔍
-3% - +3%
-1.91ms - +2.07ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
31.29ms - 33.29ms-unsure 🔍
-2% - +5%
-0.58ms - +1.67ms
unsure 🔍
-1% - +6%
-0.34ms - +1.84ms
tip-of-tree
tip-of-tree
31.22ms - 32.27msunsure 🔍
-5% - +2%
-1.67ms - +0.58ms
-unsure 🔍
-2% - +3%
-0.48ms - +0.89ms
previous-release
previous-release
31.10ms - 31.98msunsure 🔍
-6% - +1%
-1.84ms - +0.34ms
unsure 🔍
-3% - +2%
-0.89ms - +0.48ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
509.31ms - 519.47ms-unsure 🔍
-2% - +1%
-10.62ms - +6.02ms
unsure 🔍
-1% - +2%
-4.16ms - +9.65ms
tip-of-tree
tip-of-tree
510.10ms - 523.27msunsure 🔍
-1% - +2%
-6.02ms - +10.62ms
-unsure 🔍
-1% - +3%
-3.03ms - +13.13ms
previous-release
previous-release
506.96ms - 516.31msunsure 🔍
-2% - +1%
-9.65ms - +4.16ms
unsure 🔍
-3% - +1%
-13.13ms - +3.03ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
514.70ms - 526.11ms-unsure 🔍
-1% - +2%
-3.74ms - +9.93ms
unsure 🔍
-1% - +2%
-7.65ms - +7.82ms
tip-of-tree
tip-of-tree
513.55ms - 521.07msunsure 🔍
-2% - +1%
-9.93ms - +3.74ms
-unsure 🔍
-2% - +1%
-9.45ms - +3.43ms
previous-release
previous-release
515.09ms - 525.54msunsure 🔍
-2% - +1%
-7.82ms - +7.65ms
unsure 🔍
-1% - +2%
-3.43ms - +9.45ms
-

tachometer-reporter-action v2 for Benchmarks

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

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

Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very quick initial look.
Is it worth finding ways to reduce the number of type assertions?

Also love the idea of having one great analyzer that we can share.

export const parseLitTemplate = (
node: ts.TaggedTemplateExpression,
ts: TypeScript,
_checker: ts.TypeChecker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth removing as not used currently?

@@ -11,8 +11,24 @@
*/

import type ts from 'typescript';
import {_$LH} from 'lit-html/private-ssr-support.js';
import {parseFragment} from 'parse5';
// import type {Attribute} from 'parse5/dist/common/token.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intended to leave in?

const values = ts.isNoSubstitutionTemplateLiteral(node.template)
? []
: node.template.templateSpans.map((s) => s.expression);
const parts: Array<PartInfo> = [];
Copy link
Member

@kevinpschaaf kevinpschaaf Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to clarify the use cases for the API's here. i.e. whether we intend the DOM AST that comes out to be mutable or not, since the parts list will only be the "as-parsed" values.

This could either be made a getter that reifies the parts list each access (slower), or could add an "invalidateParts" API?

And are you planning on adding something to go from a LitTemplate back to a ts.TaggedTemplateExpression?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's obv fine for some use cases, where you just want to statically analyze only, but for others the parts sort of seems not that useful.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw in the lint side of things, we don't need to mutate nodes but we do need a way to translate between nodes or positions. i.e. we need a way to go for a parse5 node to an estree node (whether by reference or just by knowing the position)


type TypeScript = typeof ts;
// TODO (justinfagnani): use the real type from parse5?
type Attribute = Element['attrs'][number];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can probably import Token from parse5 and use Token.Attribute

@justinfagnani justinfagnani changed the title Add template parser to analyzer [labs/analyzer] Add template parser to analyzer Feb 1, 2024
Base automatically changed from analyzer-templates to main February 1, 2024 17:34
@justinfagnani justinfagnani force-pushed the analyzer-template-2 branch 3 times, most recently from a853ce1 to ec459b9 Compare February 2, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants