[labs/analyzer] Don't export (most) helper methods, refactor onto model classes by kevinpschaaf · Pull Request #3254 · lit/lit

kevinpschaaf · 2022-08-30T16:29:13Z

Fixes #3234 by ensuring that consumers of an analyzer model don't use incompatible helper methods (due to instanceof, caching, etc.) from a separate installation of the analyzer library.

Restricts package exports to index.js, and only exports the Analyzer class + types (and select cross-package-safe helpers). Helpers like isLitElementDeclaration and getModulesWithDeclarations are now class methods on model classes, rather than standalone module exports.

Fixes #3235 by ensuring that consumers of an analyzer model don't use incompatible helper methods (due to instanceof, caching, etc.) from a separate installation of the analyzer library. Restricts package exports to index.js, and only exports the Analyzer class + types (and select cross-package-safe helpers). Helpers like `isLitElementDeclaration` and `getModulesWithDeclarations` are now class methods on model classes, rather than standalone module exports.

changeset-bot · 2022-08-30T16:29:16Z

🦋 Changeset detected

Latest commit: 9cacac2

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

This PR includes changesets to release 6 packages

Name	Type
@lit-labs/analyzer	Minor
@lit-labs/gen-wrapper-angular	Minor
@lit-labs/gen-wrapper-react	Minor
@lit-labs/gen-wrapper-vue	Minor
@lit-labs/cli	Patch
@lit-labs/gen-utils	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 · 2022-08-30T16:29:40Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -9% - +12% (-2.49ms - +3.19ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 70.87ms - 72.11ms
lit-html-kitchen-sink: unsure 🔍 -5% - +6% (-1.52ms - +1.69ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -4% - +0% (-0.42ms - +0.04ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +0% (-1.33ms - +0.09ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +1% (-0.73ms - +0.71ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 681.85ms - 686.59ms
lit-html-kitchen-sink: unsure 🔍 -3% - +12% (-2.05ms - +10.04ms)
^{this-change vs tip-of-tree}
lit-html-repeat: faster ✔ 0% - 3% (0.16ms - 7.20ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +1% (-1.52ms - +0.89ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +0% (-4.31ms - +2.69ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 694.26ms - 698.80ms
reactive-element-list: unsure 🔍 -1% - +2% (-10.82ms - +11.96ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 105.0.5195.102
Sample size: 50
Built by: Benchmarks #3767
Commit: c37f68a

render

Version	Avg time	vs
	70.87ms - 72.11ms	-

update

Version	Avg time	vs
	681.85ms - 686.59ms	-

update-reflect

Version	Avg time	vs
	694.26ms - 698.80ms	-

lit-html-kitchen-sink

Browser: chrome-headless 105.0.5195.102
Sample size: 30
Built by: Benchmarks #3767
Commit: c37f68a

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	27.90ms - 30.04ms	-	unsure 🔍 -5% - +6% -1.52ms - +1.69ms	unsure 🔍 -4% - +5% -1.11ms - +1.52ms
tip-of-tree tip-of-tree	27.69ms - 30.08ms	unsure 🔍 -6% - +5% -1.69ms - +1.52ms	-	unsure 🔍 -5% - +5% -1.30ms - +1.54ms
previous-release previous-release	27.99ms - 29.53ms	unsure 🔍 -5% - +4% -1.52ms - +1.11ms	unsure 🔍 -5% - +4% -1.54ms - +1.30ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	82.18ms - 92.14ms	-	unsure 🔍 -3% - +12% -2.05ms - +10.04ms	unsure 🔍 -6% - +8% -5.15ms - +7.08ms
tip-of-tree tip-of-tree	79.74ms - 86.59ms	unsure 🔍 -11% - +2% -10.04ms - +2.05ms	-	unsure 🔍 -9% - +2% -7.96ms - +1.90ms
previous-release previous-release	82.64ms - 89.74ms	unsure 🔍 -8% - +6% -7.08ms - +5.15ms	unsure 🔍 -2% - +10% -1.90ms - +7.96ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	24.99ms - 29.22ms	-	unsure 🔍 -9% - +12% -2.49ms - +3.19ms	unsure 🔍 -14% - +10% -3.84ms - +2.87ms
tip-of-tree tip-of-tree	24.87ms - 28.64ms	unsure 🔍 -12% - +9% -3.19ms - +2.49ms	-	unsure 🔍 -14% - +8% -4.05ms - +2.38ms
previous-release previous-release	24.99ms - 30.19ms	unsure 🔍 -11% - +14% -2.87ms - +3.84ms	unsure 🔍 -9% - +15% -2.38ms - +4.05ms	-

lit-html-repeat

Browser: chrome-headless 105.0.5195.102
Sample size: 30
Built by: Benchmarks #3767
Commit: c37f68a

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.02ms - 10.31ms	-	unsure 🔍 -4% - +0% -0.42ms - +0.04ms	unsure 🔍 -3% - +1% -0.30ms - +0.15ms
tip-of-tree tip-of-tree	10.18ms - 10.53ms	unsure 🔍 -0% - +4% -0.04ms - +0.42ms	-	unsure 🔍 -1% - +4% -0.13ms - +0.37ms
previous-release previous-release	10.07ms - 10.41ms	unsure 🔍 -2% - +3% -0.15ms - +0.30ms	unsure 🔍 -4% - +1% -0.37ms - +0.13ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	262.92ms - 266.69ms	-	faster ✔ 0% - 3% 0.16ms - 7.20ms	unsure 🔍 -1% - +1% -2.89ms - +1.99ms
tip-of-tree tip-of-tree	265.51ms - 271.46ms	slower ❌ 0% - 3% 0.16ms - 7.20ms	-	unsure 🔍 -0% - +2% -0.13ms - +6.59ms
previous-release previous-release	263.70ms - 266.81ms	unsure 🔍 -1% - +1% -1.99ms - +2.89ms	unsure 🔍 -2% - +0% -6.59ms - +0.13ms	-

lit-html-template-heavy

Browser: chrome-headless 105.0.5195.102
Sample size: 50
Built by: Benchmarks #3767
Commit: c37f68a

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	52.64ms - 53.59ms	-	unsure 🔍 -2% - +0% -1.33ms - +0.09ms	unsure 🔍 -0% - +2% -0.22ms - +1.04ms
tip-of-tree tip-of-tree	53.21ms - 54.25ms	unsure 🔍 -0% - +3% -0.09ms - +1.33ms	-	slower ❌ 1% - 3% 0.36ms - 1.69ms
previous-release previous-release	52.29ms - 53.12ms	unsure 🔍 -2% - +0% -1.04ms - +0.22ms	faster ✔ 1% - 3% 0.36ms - 1.69ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	114.92ms - 116.69ms	-	unsure 🔍 -1% - +1% -1.52ms - +0.89ms	unsure 🔍 -1% - +1% -1.20ms - +1.56ms
tip-of-tree tip-of-tree	115.30ms - 116.94ms	unsure 🔍 -1% - +1% -0.89ms - +1.52ms	-	unsure 🔍 -1% - +2% -0.84ms - +1.83ms
previous-release previous-release	114.57ms - 116.68ms	unsure 🔍 -1% - +1% -1.56ms - +1.20ms	unsure 🔍 -2% - +1% -1.83ms - +0.84ms	-

reactive-element-list

Browser: chrome-headless 105.0.5195.102
Sample size: 50
Built by: Benchmarks #3767
Commit: c37f68a

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.01ms - 51.81ms	-	unsure 🔍 -1% - +1% -0.73ms - +0.71ms	unsure 🔍 -1% - +1% -0.77ms - +0.48ms
tip-of-tree tip-of-tree	50.82ms - 52.02ms	unsure 🔍 -1% - +1% -0.71ms - +0.73ms	-	unsure 🔍 -2% - +1% -0.91ms - +0.63ms
previous-release previous-release	51.08ms - 52.03ms	unsure 🔍 -1% - +1% -0.48ms - +0.77ms	unsure 🔍 -1% - +2% -0.63ms - +0.91ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	692.59ms - 697.72ms	-	unsure 🔍 -1% - +0% -4.31ms - +2.69ms	unsure 🔍 -0% - +1% -3.14ms - +4.24ms
tip-of-tree tip-of-tree	693.59ms - 698.35ms	unsure 🔍 -0% - +1% -2.69ms - +4.31ms	-	unsure 🔍 -0% - +1% -2.20ms - +4.93ms
previous-release previous-release	691.95ms - 697.26ms	unsure 🔍 -1% - +0% -4.24ms - +3.14ms	unsure 🔍 -1% - +0% -4.93ms - +2.20ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	786.62ms - 801.28ms	-	unsure 🔍 -1% - +2% -10.82ms - +11.96ms	unsure 🔍 -2% - +1% -12.38ms - +7.76ms
tip-of-tree tip-of-tree	784.65ms - 802.10ms	unsure 🔍 -2% - +1% -11.96ms - +10.82ms	-	unsure 🔍 -2% - +1% -14.01ms - +8.24ms
previous-release previous-release	789.35ms - 803.16ms	unsure 🔍 -1% - +2% -7.76ms - +12.38ms	unsure 🔍 -1% - +2% -8.24ms - +14.01ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2022-08-30T16:45:59Z

package.json

    "lint-staged": "^13.0.3",
    "mocha": "^10.0.0",
-    "prettier": "^2.4.1",
+    "prettier": "^2.7.1",


Is this needed in this PR? Does it change formatting and if so was that applied to all packages?

I tried updating it because the formatter was just breaking a certain code construct by inserting a random semicolon. I couldn't reproduce it in the prettier REPL, and ended up just changing the code.

We apparently don't include format in CI anymore, because (1) there were a couple of files that weren't formatted, and (2) there were a couple of generated files that were errantly not being excluded from lint (npm run format runs lint first, for some reason). Outside of those changes, I did run format and it doesn't change anything.

packages/labs/analyzer/package.json

justinfagnani · 2022-08-30T16:47:43Z

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

 ): ClassDeclaration => {
  return new ClassDeclaration({
-    name: declaration.name?.text,
+    name: declaration.name?.text ?? '',


I think I like name being undefined if a class actually doesn't have a name. What was this change for?

This change makes all Declarations now extend a common base class that has name: string (before some were string | undefined). Ultimately I think this is correct since I don't think we need any anonymous declarations in the model -- if an anonymous class/function expression is assigned to a const, we'll use that name in the declaration (I have later versions that do that). Once we support that, this will go away, so I'll add a TODO.

We could have something like:

customElements.define('my-element, class extends LitElement { ... });

Rare, but possible.

justinfagnani · 2022-08-30T16:49:14Z

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

 import {IPackageJson as PackageJson} from 'package-json-type';
 export {PackageJson};

+export type ModuleWithDeclarations<T extends Declaration> = {


Module already has declarations. Can you add a comment on what this is for? To narrow the declaration type?

Yeah, this is the return value from getModulesWithDeclarations, which returns a filtered list of modules containing a certain declaration type AND the filtered list of those declarations (in order to implement getLitElementModules()). This was probably premature factoring, I'll just revert this to getLitElementModules(); I can't really think of a use case for getting modules filtered by a different declaration type.

justinfagnani · 2022-08-30T16:50:15Z

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

    this.modules = init.modules;
  }
+
+  getModulesWithDeclarations<T extends Declaration>(


Removed, and commented getLitElementModules

justinfagnani · 2022-08-30T16:52:03Z

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

+  constructor(init: DeclarationInit) {
+    this.name = init.name;
+  }
+  isVariableDeclaration(): this is VariableDeclaration {


If we followed the module object pattern like TypeScript, we could also have a bunch of top-level functions for model object, like analyzer.isClassDeclaration(), etc.

sorvell · 2022-09-07T23:40:35Z

packages/labs/analyzer/src/index.ts

+
+// Any non-type exports below must be safe to use on objects between multiple
+// versions of the analyzer library
+export {getImportsStringForReferences} from './lib/model.js';


As a follow on, let's consider putting this on the analyzer itself and passing that around. That way we don't have to be careful about this.

sorvell

LGTM, just one comment to handle as a follow on.

justinfagnani

Still LGTM!

packages/labs/analyzer/package.json

justinfagnani · 2022-09-08T00:16:05Z

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

 ): ClassDeclaration => {
  return new ClassDeclaration({
-    name: declaration.name?.text,
+    name: declaration.name?.text ?? '',


We could have something like:

customElements.define('my-element, class extends LitElement { ... });

Rare, but possible.

kevinpschaaf requested a review from justinfagnani as a code owner August 30, 2022 16:29

Add changeset

d425dbe

justinfagnani approved these changes Aug 30, 2022

View reviewed changes

kevinpschaaf changed the title ~~Don't export (most) helper methods, refactor onto model classes~~ [labs/analyzer] Don't export (most) helper methods, refactor onto model classes Aug 31, 2022

kevinpschaaf added 4 commits September 6, 2022 09:21

Run format and fix formatting config issues

346b979

Document getLitElementModules and remove general version

75b87d2

Add TODO

e588bf9

Merge branch 'main' into analyzer-isLitElement

9cacac2

sorvell reviewed Sep 7, 2022

View reviewed changes

sorvell approved these changes Sep 7, 2022

View reviewed changes

justinfagnani approved these changes Sep 8, 2022

View reviewed changes

kevinpschaaf merged commit fc2fd4c into main Sep 8, 2022

kevinpschaaf deleted the analyzer-isLitElement branch September 8, 2022 00:43

This was referenced Oct 3, 2022

Version Packages #3328

Closed

Version Packages #3335

Closed

Version Packages #3337

Merged

Conversation

kevinpschaaf commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Aug 30, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sorvell left a comment

Choose a reason for hiding this comment

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinpschaaf commented Aug 30, 2022 •

edited

Loading

changeset-bot bot commented Aug 30, 2022 •

edited

Loading

github-actions bot commented Aug 30, 2022 •

edited

Loading