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

[reactive-element] New queryAssignedNodes API and deprecate current API #2338

Merged
merged 5 commits into from
Dec 8, 2021

Conversation

AndrewJakubowicz
Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz commented Dec 7, 2021

Addresses: #2332 and #2171

Following the inclusion of queryAssignedElements, this change adds a new API and deprecates the old @queryAssignedNodes API.

Note the new queryAssignedNodes options does not include selector. If using selector please use @queryAssignedElements. @queryAssignedNodes('', false, '.item') is functionally identical to @queryAssignedElements({slot: '', flatten: false, selector: '.item'}) or @queryAssignedElements({selector: '.item'}).

Features

  • Updated the interface in both TypeScript and jsdocs to deprecate the previous API, and document the new API. The deprecated API contains information about upgrading to use the new API and also mentions @queryAssignedElements.
  • @queryAssignedNodes supports both APIs.
  • Transformers:
    • queryAssignedNodes visitor refactored slightly so that it continues to work on legacy documentation examples.
    • queryAssignedElements visitor refactored so it can be easily extended. It works almost exactly the same as queryAssignedElements but with assignedNodes method and the generated selector modified.
  • Manually tested lit.dev API documentation generation and ensured that examples render correctly for both @queryAssignedElements and @queryAssignedNodes.

Testing

  • VS Code editor experience tested manually (and with tests). Both APIs are tested and documented, and the deprecated API gets marked or crossed out by the IDE.
    Screen Shot 2021-12-06 at 5 44 30 PM
  • Functional unit tests and ts-transformer tests.
  • Manually tested and built Lit.dev to check that it renders the correct overloading documentation.

Manual testing screenshots on Lit.dev API

Screenshots showing the fixed @queryAssignedElements and @queryAssignedNodes generated API.

Screen Shot 2021-12-07 at 1 05 14 PM

Screen Shot 2021-12-07 at 1 05 21 PM

Screen Shot 2021-12-07 at 1 05 30 PM

Screen Shot 2021-12-07 at 1 05 38 PM

@changeset-bot
Copy link

changeset-bot bot commented Dec 7, 2021

🦋 Changeset detected

Latest commit: ef9216d

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

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

@AndrewJakubowicz AndrewJakubowicz changed the base branch from main to feat-query-assigned-elements December 7, 2021 01:43
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2021

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -2% - +1% (-0.52ms - +0.32ms)
    this-change vs tip-of-tree

render

  • lit-element-list: unsure 🔍 -1% - +1% (-0.95ms - +1.24ms)
    this-change vs tip-of-tree
  • lit-html-kitchen-sink: unsure 🔍 -1% - +0% (-0.34ms - +0.16ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -3% - +1% (-0.42ms - +0.18ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -4% - +0% (-2.41ms - +0.30ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -2% - +2% (-1.56ms - +1.00ms)
    this-change vs tip-of-tree

update

  • lit-element-list: unsure 🔍 -1% - +1% (-6.94ms - +7.78ms)
    this-change vs tip-of-tree
  • lit-html-kitchen-sink: unsure 🔍 -1% - +3% (-0.75ms - +3.13ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -7% - +7% (-27.27ms - +25.71ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -1% - +1% (-2.12ms - +1.21ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -1% - +1% (-10.64ms - +5.40ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: unsure 🔍 -1% - +0% (-8.30ms - +4.88ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -1% - +0% (-11.11ms - +4.60ms)
    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
89.03ms - 90.51ms-unsure 🔍
-1% - +1%
-0.95ms - +1.24ms
faster ✔
20% - 22%
23.24ms - 25.68ms
tip-of-tree
tip-of-tree
88.82ms - 90.43msunsure 🔍
-1% - +1%
-1.24ms - +0.95ms
-faster ✔
21% - 23%
23.34ms - 25.86ms
previous-release
previous-release
113.26ms - 115.20msslower ❌
26% - 29%
23.24ms - 25.68ms
slower ❌
26% - 29%
23.34ms - 25.86ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
907.78ms - 916.84ms-unsure 🔍
-1% - +1%
-6.94ms - +7.78ms
faster ✔
7% - 8%
68.41ms - 84.03ms
tip-of-tree
tip-of-tree
906.09ms - 917.68msunsure 🔍
-1% - +1%
-7.78ms - +6.94ms
-faster ✔
7% - 9%
68.04ms - 85.25ms
previous-release
previous-release
982.17ms - 994.89msslower ❌
7% - 9%
68.41ms - 84.03ms
slower ❌
7% - 9%
68.04ms - 85.25ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
1005.05ms - 1013.52ms-unsure 🔍
-1% - +0%
-8.30ms - +4.88ms
faster ✔
4% - 5%
39.25ms - 53.60ms
tip-of-tree
tip-of-tree
1005.95ms - 1016.05msunsure 🔍
-0% - +1%
-4.88ms - +8.30ms
-faster ✔
4% - 5%
37.03ms - 52.40ms
previous-release
previous-release
1049.92ms - 1061.50msslower ❌
4% - 5%
39.25ms - 53.60ms
slower ❌
4% - 5%
37.03ms - 52.40ms
-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
35.37ms - 35.66ms-unsure 🔍
-1% - +0%
-0.34ms - +0.16ms
faster ✔
15% - 18%
6.25ms - 7.52ms
tip-of-tree
tip-of-tree
35.40ms - 35.80msunsure 🔍
-0% - +1%
-0.16ms - +0.34ms
-faster ✔
15% - 17%
6.15ms - 7.45ms
previous-release
previous-release
41.78ms - 43.02msslower ❌
18% - 21%
6.25ms - 7.52ms
slower ❌
17% - 21%
6.15ms - 7.45ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
99.99ms - 102.82ms-unsure 🔍
-1% - +3%
-0.75ms - +3.13ms
faster ✔
0% - 6%
0.04ms - 6.34ms
tip-of-tree
tip-of-tree
98.89ms - 101.55msunsure 🔍
-3% - +1%
-3.13ms - +0.75ms
-faster ✔
1% - 7%
1.27ms - 7.49ms
previous-release
previous-release
101.79ms - 107.41msslower ❌
0% - 6%
0.04ms - 6.34ms
slower ❌
1% - 7%
1.27ms - 7.49ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
30.25ms - 30.80ms-unsure 🔍
-2% - +1%
-0.52ms - +0.32ms
faster ✔
13% - 16%
4.65ms - 5.72ms
tip-of-tree
tip-of-tree
30.31ms - 30.94msunsure 🔍
-1% - +2%
-0.32ms - +0.52ms
-faster ✔
13% - 16%
4.52ms - 5.65ms
previous-release
previous-release
35.25ms - 36.18msslower ❌
15% - 19%
4.65ms - 5.72ms
slower ❌
15% - 19%
4.52ms - 5.65ms
-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
13.25ms - 13.67ms-unsure 🔍
-3% - +1%
-0.42ms - +0.18ms
faster ✔
6% - 9%
0.90ms - 1.34ms
tip-of-tree
tip-of-tree
13.37ms - 13.79msunsure 🔍
-1% - +3%
-0.18ms - +0.42ms
-faster ✔
5% - 8%
0.78ms - 1.22ms
previous-release
previous-release
14.52ms - 14.64msslower ❌
7% - 10%
0.90ms - 1.34ms
slower ❌
6% - 9%
0.78ms - 1.22ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
368.97ms - 406.28ms-unsure 🔍
-7% - +7%
-27.27ms - +25.71ms
faster ✔
26% - 35%
138.36ms - 196.96ms
tip-of-tree
tip-of-tree
369.59ms - 407.22msunsure 🔍
-7% - +7%
-25.71ms - +27.27ms
-faster ✔
26% - 34%
137.48ms - 196.29ms
previous-release
previous-release
532.69ms - 577.88msslower ❌
34% - 52%
138.36ms - 196.96ms
slower ❌
34% - 52%
137.48ms - 196.29ms
-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
65.00ms - 66.31ms-unsure 🔍
-4% - +0%
-2.41ms - +0.30ms
faster ✔
15% - 18%
11.55ms - 13.88ms
tip-of-tree
tip-of-tree
65.52ms - 67.89msunsure 🔍
-0% - +4%
-0.30ms - +2.41ms
-faster ✔
13% - 17%
10.13ms - 13.19ms
previous-release
previous-release
77.40ms - 79.33msslower ❌
17% - 21%
11.55ms - 13.88ms
slower ❌
15% - 20%
10.13ms - 13.19ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
147.80ms - 149.91ms-unsure 🔍
-1% - +1%
-2.12ms - +1.21ms
faster ✔
12% - 14%
19.72ms - 23.73ms
tip-of-tree
tip-of-tree
148.03ms - 150.60msunsure 🔍
-1% - +1%
-1.21ms - +2.12ms
-faster ✔
11% - 14%
19.14ms - 23.40ms
previous-release
previous-release
168.88ms - 172.29msslower ❌
13% - 16%
19.72ms - 23.73ms
slower ❌
13% - 16%
19.14ms - 23.40ms
-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
63.46ms - 64.98ms-unsure 🔍
-2% - +2%
-1.56ms - +1.00ms
unsure 🔍
-1% - +3%
-0.61ms - +1.68ms
tip-of-tree
tip-of-tree
63.47ms - 65.52msunsure 🔍
-2% - +2%
-1.00ms - +1.56ms
-unsure 🔍
-1% - +3%
-0.53ms - +2.15ms
previous-release
previous-release
62.83ms - 64.54msunsure 🔍
-3% - +1%
-1.68ms - +0.61ms
unsure 🔍
-3% - +1%
-2.15ms - +0.53ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
941.54ms - 954.18ms-unsure 🔍
-1% - +1%
-10.64ms - +5.40ms
unsure 🔍
-1% - +1%
-9.71ms - +7.28ms
tip-of-tree
tip-of-tree
945.55ms - 955.41msunsure 🔍
-1% - +1%
-5.40ms - +10.64ms
-unsure 🔍
-1% - +1%
-6.11ms - +8.92ms
previous-release
previous-release
943.40ms - 954.75msunsure 🔍
-1% - +1%
-7.28ms - +9.71ms
unsure 🔍
-1% - +1%
-8.92ms - +6.11ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
1061.68ms - 1073.08ms-unsure 🔍
-1% - +0%
-11.11ms - +4.60ms
unsure 🔍
-1% - +0%
-11.17ms - +4.07ms
tip-of-tree
tip-of-tree
1065.23ms - 1076.04msunsure 🔍
-0% - +1%
-4.60ms - +11.11ms
-unsure 🔍
-1% - +1%
-7.71ms - +7.12ms
previous-release
previous-release
1065.86ms - 1076.00msunsure 🔍
-0% - +1%
-4.07ms - +11.17ms
unsure 🔍
-1% - +1%
-7.12ms - +7.71ms
-

tachometer-reporter-action v2 for Benchmarks

@justinfagnani justinfagnani changed the title New queryAssignedNodes API and deprecate current API [reactive-element] New queryAssignedNodes API and deprecate current API Dec 7, 2021
@justinfagnani justinfagnani added this to the Lit 2.1 milestone Dec 7, 2021
Copy link
Member

@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.

Transformer part of this LGTM, great stuff!

property: ts.ClassElement,
decorator: ts.Decorator
) {
if (this.legacyVisitor.visit(litClassContext, property, decorator)) {
Copy link
Member

Choose a reason for hiding this comment

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

Clever!

Base automatically changed from feat-query-assigned-elements to main December 7, 2021 18:18
…ecated legacy API.

This brings the queryAssignedNodes API to a symmetry with the @queryAssignedElements API.
Minor refactoring has been done so that the legacy assignedNodes
visitor can be run ahead of the extended queryAssignedElementsVisitor
which was slightly refactored.
@AndrewJakubowicz AndrewJakubowicz force-pushed the query-assigned-nodes-api branch 3 times, most recently from 79db184 to 3a71b8d Compare December 7, 2021 20:32
@AndrewJakubowicz AndrewJakubowicz marked this pull request as ready for review December 7, 2021 21:14
@justinfagnani
Copy link
Collaborator

Do our docs not show overloads, or do they not show deprecated overloads?

@AndrewJakubowicz
Copy link
Contributor Author

Do our docs not show overloads, or do they not show deprecated overloads?

Our generated API documentation does not show overloads.

Also removed the object creation and destructuring method of normalizing
arguments.
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