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

[transformers ] Add @lit/ts-transformers package with idiomatic decorator transform #1919

Merged
merged 42 commits into from
Jul 30, 2021

Conversation

aomarks
Copy link
Member

@aomarks aomarks commented May 21, 2021

Adds a @lit/ts-transformers package to contain TypeScript transforms.

This PR starts with the "idiomatic" decorator tansform, which transforms all Lit decorators into JavaScript idiomatic style.

The initial motivating use case for this transform is to automatically generate https://lit.dev examples in non-decorator JavaScript, controlled by a JS/TS switch (lit/lit.dev#332).

There may also be some size/performance improvements by using this transform, but we'll want to benchmark before saying that -- plus there's more we can do for this goal, such as defining getters/setters for reactive properties too.

Fixes #1886

Example

Before

import {LitElement, html} from 'lit';
import {
  customElement,
  property,
  state,
  query,
  eventOptions,
} from 'lit/decorators.js';

@customElement('simple-greeting')
class SimpleGreeting extends LitElement {
  @property()
  name = 'Somebody';

  @state()
  private counter = 0;

  @query('#myButton')
  button?: HTMLButtonElement;

  render() {
    return html`
      <button id="myButton" @click=${this._onClick}>
        Hello ${this.name} (${this.counter})
      </button>
    `;
  }

  @eventOptions({capture: true})
  _onClick(event: Event) {
    this.counter++;
    console.log('click', event.target);
  }
}

After

import {LitElement, html} from 'lit';

class MyElement extends LitElement {
  static get properties() {
    return {
      str: {},
      num: {state: true},
    };
  }

  get button() {
    return this.renderRoot?.querySelector('#myButton');
  }

  constructor() {
    super(...arguments);
    this.name = 'Somebody';
    this.counter = 0;
    this._onClick = {
      handleEvent: (event) => {
        console.log('click', event.target);
      },
      capture: true,
    };
  }

  render() {
    return html`
      <button id="myButton" @click=${this._onClick}>
        Hello ${this.name} (${this.counter})
      </button>
    `;
  }
}

customElements.define('simple-greeting', SimpleGreeting);

@aomarks aomarks requested a review from justinfagnani May 21, 2021 00:07
@google-cla google-cla bot added the cla: yes label May 21, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 21, 2021

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -5% - +4% (-2.12ms - +1.80ms)
    this-change vs tip-of-tree

render

  • lit-element-list: unsure 🔍 -5% - +1% (-6.43ms - +1.78ms)
    this-change vs tip-of-tree
  • lit-html-kitchen-sink: unsure 🔍 -3% - +5% (-1.66ms - +2.75ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -4% - +6% (-0.66ms - +0.91ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -1% - +4% (-1.03ms - +3.21ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -6% - +5% (-4.81ms - +3.83ms)
    this-change vs tip-of-tree

update

  • lit-element-list: unsure 🔍 -2% - +1% (-22.43ms - +11.64ms)
    this-change vs tip-of-tree
  • lit-html-kitchen-sink: unsure 🔍 -6% - +4% (-8.18ms - +6.01ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -2% - +2% (-10.56ms - +9.21ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -2% - +2% (-3.58ms - +4.29ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -2% - +1% (-21.95ms - +8.36ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: unsure 🔍 -1% - +3% (-10.56ms - +29.73ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -2% - +2% (-16.71ms - +23.37ms)
    this-change vs tip-of-tree

Results

lit-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
117.43ms - 123.55ms-unsure 🔍
-5% - +1%
-6.43ms - +1.78ms
faster ✔
23% - 28%
37.51ms - 47.06ms
tip-of-tree
tip-of-tree
120.07ms - 125.56msunsure 🔍
-2% - +5%
-1.78ms - +6.43ms
-faster ✔
22% - 27%
35.38ms - 44.54ms
previous-release
previous-release
159.10ms - 166.45msslower ❌
31% - 40%
37.51ms - 47.06ms
slower ❌
28% - 37%
35.38ms - 44.54ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
995.93ms - 1020.08ms-unsure 🔍
-2% - +1%
-22.43ms - +11.64ms
faster ✔
7% - 9%
70.64ms - 104.75ms
tip-of-tree
tip-of-tree
1001.38ms - 1025.42msunsure 🔍
-1% - +2%
-11.64ms - +22.43ms
-faster ✔
6% - 9%
65.28ms - 99.31ms
previous-release
previous-release
1083.65ms - 1107.74msslower ❌
7% - 10%
70.64ms - 104.75ms
slower ❌
6% - 10%
65.28ms - 99.31ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
1040.38ms - 1069.46ms-unsure 🔍
-1% - +3%
-10.56ms - +29.73ms
faster ✔
2% - 5%
17.07ms - 54.80ms
tip-of-tree
tip-of-tree
1031.39ms - 1059.27msunsure 🔍
-3% - +1%
-29.73ms - +10.56ms
-faster ✔
3% - 6%
27.12ms - 63.92ms
previous-release
previous-release
1078.84ms - 1102.87msslower ❌
2% - 5%
17.07ms - 54.80ms
slower ❌
3% - 6%
27.12ms - 63.92ms
-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
49.76ms - 53.11ms-unsure 🔍
-3% - +5%
-1.66ms - +2.75ms
faster ✔
14% - 20%
8.46ms - 12.78ms
tip-of-tree
tip-of-tree
49.46ms - 52.32msunsure 🔍
-5% - +3%
-2.75ms - +1.66ms
-faster ✔
15% - 21%
9.19ms - 13.14ms
previous-release
previous-release
60.69ms - 63.42msslower ❌
16% - 25%
8.46ms - 12.78ms
slower ❌
18% - 26%
9.19ms - 13.14ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
132.74ms - 141.40ms-unsure 🔍
-6% - +4%
-8.18ms - +6.01ms
faster ✔
0% - 9%
0.03ms - 13.73ms
tip-of-tree
tip-of-tree
132.54ms - 143.77msunsure 🔍
-4% - +6%
-6.01ms - +8.18ms
-unsure 🔍
-9% - +1%
-13.52ms - +1.94ms
previous-release
previous-release
138.64ms - 149.26msunsure 🔍
-0% - +10%
+0.03ms - +13.73ms
unsure 🔍
-2% - +10%
-1.94ms - +13.52ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
44.01ms - 46.62ms-unsure 🔍
-5% - +4%
-2.12ms - +1.80ms
slower ❌
1% - 10%
0.42ms - 4.29ms
tip-of-tree
tip-of-tree
44.01ms - 46.94msunsure 🔍
-4% - +5%
-1.80ms - +2.12ms
-slower ❌
1% - 11%
0.47ms - 4.56ms
previous-release
previous-release
41.54ms - 44.38msfaster ✔
1% - 9%
0.42ms - 4.29ms
faster ✔
1% - 10%
0.47ms - 4.56ms
-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
14.51ms - 15.88ms-unsure 🔍
-4% - +6%
-0.66ms - +0.91ms
faster ✔
11% - 20%
1.89ms - 3.62ms
tip-of-tree
tip-of-tree
14.69ms - 15.45msunsure 🔍
-6% - +4%
-0.91ms - +0.66ms
-faster ✔
13% - 19%
2.23ms - 3.52ms
previous-release
previous-release
17.43ms - 18.47msslower ❌
12% - 24%
1.89ms - 3.62ms
slower ❌
14% - 24%
2.23ms - 3.52ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
460.24ms - 473.82ms-unsure 🔍
-2% - +2%
-10.56ms - +9.21ms
faster ✔
28% - 32%
186.32ms - 216.13ms
tip-of-tree
tip-of-tree
460.52ms - 474.89msunsure 🔍
-2% - +2%
-9.21ms - +10.56ms
-faster ✔
28% - 32%
185.47ms - 215.64ms
previous-release
previous-release
654.99ms - 681.52msslower ❌
40% - 47%
186.32ms - 216.13ms
slower ❌
39% - 46%
185.47ms - 215.64ms
-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
81.09ms - 84.01ms-unsure 🔍
-1% - +4%
-1.03ms - +3.21ms
faster ✔
14% - 18%
13.79ms - 17.43ms
tip-of-tree
tip-of-tree
79.92ms - 82.99msunsure 🔍
-4% - +1%
-3.21ms - +1.03ms
-faster ✔
15% - 19%
14.82ms - 18.59ms
previous-release
previous-release
97.07ms - 99.25msslower ❌
16% - 21%
13.79ms - 17.43ms
slower ❌
18% - 23%
14.82ms - 18.59ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
172.87ms - 178.14ms-unsure 🔍
-2% - +2%
-3.58ms - +4.29ms
faster ✔
9% - 12%
16.54ms - 24.14ms
tip-of-tree
tip-of-tree
172.22ms - 178.07msunsure 🔍
-2% - +2%
-4.29ms - +3.58ms
-faster ✔
9% - 13%
16.69ms - 24.71ms
previous-release
previous-release
193.11ms - 198.58msslower ❌
9% - 14%
16.54ms - 24.14ms
slower ❌
9% - 14%
16.69ms - 24.71ms
-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
83.92ms - 86.29ms-unsure 🔍
-2% - +4%
-1.48ms - +3.23ms
unsure 🔍
-2% - +2%
-2.09ms - +1.67ms
tip-of-tree
tip-of-tree
82.19ms - 86.26msunsure 🔍
-4% - +2%
-3.23ms - +1.48ms
-unsure 🔍
-4% - +2%
-3.59ms - +1.42ms
previous-release
previous-release
83.86ms - 86.77msunsure 🔍
-2% - +2%
-1.67ms - +2.09ms
unsure 🔍
-2% - +4%
-1.42ms - +3.59ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
1051.77ms - 1077.95ms-unsure 🔍
-2% - +2%
-17.11ms - +21.37ms
unsure 🔍
-2% - +2%
-18.85ms - +17.45ms
tip-of-tree
tip-of-tree
1048.63ms - 1076.83msunsure 🔍
-2% - +2%
-21.37ms - +17.11ms
-unsure 🔍
-2% - +2%
-21.72ms - +16.06ms
previous-release
previous-release
1052.99ms - 1078.13msunsure 🔍
-2% - +2%
-17.45ms - +18.85ms
unsure 🔍
-2% - +2%
-16.06ms - +21.72ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
1128.62ms - 1153.53ms-unsure 🔍
-2% - +1%
-26.65ms - +11.49ms
unsure 🔍
-2% - +1%
-24.95ms - +11.49ms
tip-of-tree
tip-of-tree
1134.21ms - 1163.10msunsure 🔍
-1% - +2%
-11.49ms - +26.65ms
-unsure 🔍
-2% - +2%
-18.78ms - +20.49ms
previous-release
previous-release
1134.50ms - 1161.10msunsure 🔍
-1% - +2%
-11.49ms - +24.95ms
unsure 🔍
-2% - +2%
-20.49ms - +18.78ms
-

tachometer-reporter-action v2 for Benchmarks

@aomarks aomarks force-pushed the transformers branch 3 times, most recently from 35ac4ff to bf208d7 Compare May 21, 2021 00:56
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/tests/idiomatic-test.ts Outdated Show resolved Hide resolved
@justinfagnani justinfagnani changed the title Add @lit/transformers package with idiomatic decorator transform [transformers ] Add @lit/transformers package with idiomatic decorator transform May 25, 2021
@kevinpschaaf
Copy link
Member

kevinpschaaf commented May 25, 2021

Suggestion for more idiomatic TS -> JS without using decorators.

<div @click=${{passive: true, handleEvent: e => this.handleClick(e)}>

@aomarks
Copy link
Member Author

aomarks commented Jun 15, 2021

Suggestion for more idiomatic TS -> JS without using decorators.

<div @click=${{passive: true, handleEvent: e => this.handleClick(e)}>

Done, as long as the method is private. Otherwise, we'll annotate the prototype method directly (in case it's e.g. used by a subclass).

@aomarks aomarks requested a review from rictic June 15, 2021 22:39
@aomarks
Copy link
Member Author

aomarks commented Jun 15, 2021

@rictic @justinfagnani @kevinpschaaf PTAL

I've addressed all comments, and made some additional improvements. Main items:

  • The @eventOptions decorator now transforms corresponding references inside Lit template event bindings to the idiomatic syntax we agreed on offline (@click=${{handleEvent: (e) => this._onClick(e), capture: true}}), as long as the method is private. If it's not private, we annotate the prototype method directly (in case e.g. it is used in an event binding by a subclass).

    Note this required a little refactoring, since we now need to conditionally transform Lit templates anywhere in the class based on this decorator. The general pattern I came up with for this is to allow visitors to spawn new visitors that will also run while we're still scoped to the class.

  • We now actually track imports from Lit modules, and check for decorators/html function with a type checker symbol lookup, instead of making naming assumptions. Works if the import is aliased, too.

  • We now skip full traversal of files if we don't see any relevant Lit imports right away.

  • A bit of refactoring/reorganization: added LitFileContext and LitClassContext classes, which track state for files/classes respectively.

@changeset-bot
Copy link

changeset-bot bot commented Jul 15, 2021

⚠️ No Changeset found

Latest commit: 823e38a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

package.json Outdated Show resolved Hide resolved
packages/transformers/package.json Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/visitor.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/tests/idiomatic-test.ts Outdated Show resolved Hide resolved
packages/transformers/src/tests/idiomatic-test.ts Outdated Show resolved Hide resolved
packages/transformers/src/tests/idiomatic-test.ts Outdated Show resolved Hide resolved
packages/transformers/src/tests/idiomatic-test.ts Outdated Show resolved Hide resolved
packages/transformers/src/tests/idiomatic-test.ts Outdated Show resolved Hide resolved
Copy link
Member Author

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

All comments addressed @justinfagnani PTAL

package.json Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/src/visitor.ts Outdated Show resolved Hide resolved
packages/transformers/src/lit-transformer.ts Outdated Show resolved Hide resolved
packages/transformers/package.json Outdated Show resolved Hide resolved
@aomarks aomarks requested a review from justinfagnani July 16, 2021 00:19
@aomarks aomarks changed the title [transformers ] Add @lit/transformers package with idiomatic decorator transform [transformers ] Add @lit/ts-transformers package with idiomatic decorator transform Jul 29, 2021
@aomarks aomarks merged commit 2f6dc41 into main Jul 30, 2021
@aomarks aomarks deleted the transformers branch July 30, 2021 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform decorators to readable JS
4 participants