[reactive-element] Add queryAssignedElements decorator #2327

AndrewJakubowicz · 2021-12-01T02:58:01Z

Adds queryAssignedElements decorator and tests (that similarly match the queryAssignedNodes tests).

This change proposes a new options bag API which lets us avoid needing to use and empty string if targeting an unnamed slot.

Note that there is temporarily an asymmetry between this decorator and queryAssignedNodes which will be addressed with #2332.

How

The first commit introduces the new decorator, with tests matching closely what the queryAssignedNodes tests. In fact the fixture is almost identical.
Then I add the imports closely following the query-assigned-nodes references in the codebase.
Finally I updated the ts-transformer such that it can transform queryAssignedElements into JS.

Testing

Change is tested with unit tests, as well as transformer tests.

changeset-bot · 2021-12-01T02:58:04Z

🦋 Changeset detected

Latest commit: b3dae4b

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

github-actions · 2021-12-01T02:58:27Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -2% - +1% (-0.49ms - +0.17ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -0% - +2% (-0.27ms - +1.22ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -1% - +0% (-0.33ms - +0.04ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -6% - +14% (-0.66ms - +1.61ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +1% (-0.79ms - +0.77ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +1% (-1.05ms - +0.56ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -0% - +1% (-2.08ms - +4.47ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -2% - +3% (-1.57ms - +2.21ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -1% - +1% (-2.31ms - +2.75ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +1% (-1.00ms - +0.84ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +1% (-1.94ms - +4.38ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -1% - +0% (-5.88ms - +1.93ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +1% (-4.20ms - +4.47ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 96.0.4664.45
Sample size: 50
Built by: Benchmarks #2096
Commit: 1568092

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	70.49ms - 71.77ms	-	unsure 🔍 -0% - +2% -0.27ms - +1.22ms	faster ✔ 21% - 22% 18.51ms - 20.51ms
tip-of-tree tip-of-tree	70.28ms - 71.03ms	unsure 🔍 -2% - +0% -1.22ms - +0.27ms	-	faster ✔ 21% - 23% 19.13ms - 20.84ms
previous-release previous-release	89.87ms - 91.41ms	slower ❌ 26% - 29% 18.51ms - 20.51ms	slower ❌ 27% - 30% 19.13ms - 20.84ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	614.62ms - 619.30ms	-	unsure 🔍 -0% - +1% -2.08ms - +4.47ms	faster ✔ 7% - 9% 49.04ms - 57.18ms
tip-of-tree tip-of-tree	613.47ms - 618.06ms	unsure 🔍 -1% - +0% -4.47ms - +2.08ms	-	faster ✔ 8% - 9% 50.26ms - 58.35ms
previous-release previous-release	666.74ms - 673.40ms	slower ❌ 8% - 9% 49.04ms - 57.18ms	slower ❌ 8% - 9% 50.26ms - 58.35ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	673.77ms - 679.05ms	-	unsure 🔍 -1% - +0% -5.88ms - +1.93ms	faster ✔ 3% - 4% 19.06ms - 26.76ms
tip-of-tree tip-of-tree	675.50ms - 681.27ms	unsure 🔍 -0% - +1% -1.93ms - +5.88ms	-	faster ✔ 2% - 4% 16.91ms - 24.95ms
previous-release previous-release	696.52ms - 702.12ms	slower ❌ 3% - 4% 19.06ms - 26.76ms	slower ❌ 2% - 4% 16.91ms - 24.95ms	-

lit-html-kitchen-sink

Browser: chrome-headless 96.0.4664.45
Sample size: 30
Built by: Benchmarks #2096
Commit: 1568092

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	28.41ms - 28.67ms	-	unsure 🔍 -1% - +0% -0.33ms - +0.04ms	faster ✔ 14% - 16% 4.57ms - 5.48ms
tip-of-tree tip-of-tree	28.55ms - 28.81ms	unsure 🔍 -0% - +1% -0.04ms - +0.33ms	-	faster ✔ 13% - 16% 4.42ms - 5.34ms
previous-release previous-release	33.12ms - 34.00ms	slower ❌ 16% - 19% 4.57ms - 5.48ms	slower ❌ 15% - 19% 4.42ms - 5.34ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	75.91ms - 79.21ms	-	unsure 🔍 -2% - +3% -1.57ms - +2.21ms	faster ✔ 2% - 7% 1.34ms - 5.90ms
tip-of-tree tip-of-tree	76.32ms - 78.15ms	unsure 🔍 -3% - +2% -2.21ms - +1.57ms	-	faster ✔ 3% - 7% 2.12ms - 5.76ms
previous-release previous-release	79.61ms - 82.74ms	slower ❌ 2% - 8% 1.34ms - 5.90ms	slower ❌ 3% - 7% 2.12ms - 5.76ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	24.09ms - 24.47ms	-	unsure 🔍 -2% - +1% -0.49ms - +0.17ms	faster ✔ 11% - 16% 3.08ms - 4.65ms
tip-of-tree tip-of-tree	24.17ms - 24.71ms	unsure 🔍 -1% - +2% -0.17ms - +0.49ms	-	faster ✔ 11% - 16% 2.90ms - 4.51ms
previous-release previous-release	27.38ms - 28.91ms	slower ❌ 13% - 19% 3.08ms - 4.65ms	slower ❌ 12% - 19% 2.90ms - 4.51ms	-

lit-html-repeat

Browser: chrome-headless 96.0.4664.45
Sample size: 30
Built by: Benchmarks #2096
Commit: 1568092

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.84ms - 13.08ms	-	unsure 🔍 -6% - +14% -0.66ms - +1.61ms	unsure 🔍 -13% - +5% -1.57ms - +0.68ms
tip-of-tree tip-of-tree	11.34ms - 11.63ms	unsure 🔍 -13% - +5% -1.61ms - +0.66ms	-	faster ✔ 6% - 9% 0.77ms - 1.07ms
previous-release previous-release	12.37ms - 12.45ms	unsure 🔍 -6% - +13% -0.68ms - +1.57ms	slower ❌ 7% - 9% 0.77ms - 1.07ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	277.51ms - 281.14ms	-	unsure 🔍 -1% - +1% -2.31ms - +2.75ms	faster ✔ 28% - 30% 111.24ms - 116.98ms
tip-of-tree tip-of-tree	277.34ms - 280.86ms	unsure 🔍 -1% - +1% -2.75ms - +2.31ms	-	faster ✔ 28% - 30% 111.50ms - 117.17ms
previous-release previous-release	391.21ms - 395.65ms	slower ❌ 40% - 42% 111.24ms - 116.98ms	slower ❌ 40% - 42% 111.50ms - 117.17ms	-

lit-html-template-heavy

Browser: chrome-headless 96.0.4664.45
Sample size: 50
Built by: Benchmarks #2096
Commit: 1568092

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.14ms - 52.48ms	-	unsure 🔍 -2% - +1% -0.79ms - +0.77ms	faster ✔ 14% - 17% 8.81ms - 10.77ms
tip-of-tree tip-of-tree	51.41ms - 52.23ms	unsure 🔍 -1% - +2% -0.77ms - +0.79ms	-	faster ✔ 15% - 17% 8.95ms - 10.61ms
previous-release previous-release	60.88ms - 62.32ms	slower ❌ 17% - 21% 8.81ms - 10.77ms	slower ❌ 17% - 21% 8.95ms - 10.61ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	107.47ms - 108.84ms	-	unsure 🔍 -1% - +1% -1.00ms - +0.84ms	faster ✔ 12% - 14% 14.86ms - 17.26ms
tip-of-tree tip-of-tree	107.62ms - 108.85ms	unsure 🔍 -1% - +1% -0.84ms - +1.00ms	-	faster ✔ 12% - 14% 14.82ms - 17.14ms
previous-release previous-release	123.24ms - 125.20ms	slower ❌ 14% - 16% 14.86ms - 17.26ms	slower ❌ 14% - 16% 14.82ms - 17.14ms	-

reactive-element-list

Browser: chrome-headless 96.0.4664.45
Sample size: 50
Built by: Benchmarks #2096
Commit: 1568092

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.40ms - 52.48ms	-	unsure 🔍 -2% - +1% -1.05ms - +0.56ms	unsure 🔍 -1% - +2% -0.27ms - +1.22ms
tip-of-tree tip-of-tree	51.59ms - 52.77ms	unsure 🔍 -1% - +2% -0.56ms - +1.05ms	-	unsure 🔍 -0% - +3% -0.06ms - +1.50ms
previous-release previous-release	50.95ms - 51.97ms	unsure 🔍 -2% - +1% -1.22ms - +0.27ms	unsure 🔍 -3% - +0% -1.50ms - +0.06ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	622.68ms - 627.10ms	-	unsure 🔍 -0% - +1% -1.94ms - +4.38ms	unsure 🔍 -0% - +1% -2.68ms - +3.72ms
tip-of-tree tip-of-tree	621.41ms - 625.92ms	unsure 🔍 -1% - +0% -4.38ms - +1.94ms	-	unsure 🔍 -1% - +0% -3.94ms - +2.54ms
previous-release previous-release	622.05ms - 626.68ms	unsure 🔍 -1% - +0% -3.72ms - +2.68ms	unsure 🔍 -0% - +1% -2.54ms - +3.94ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	700.75ms - 707.64ms	-	unsure 🔍 -1% - +1% -4.20ms - +4.47ms	unsure 🔍 -1% - +1% -5.26ms - +4.36ms
tip-of-tree tip-of-tree	701.43ms - 706.69ms	unsure 🔍 -1% - +1% -4.47ms - +4.20ms	-	unsure 🔍 -1% - +1% -4.85ms - +3.69ms
previous-release previous-release	701.28ms - 708.00ms	unsure 🔍 -1% - +1% -4.36ms - +5.26ms	unsure 🔍 -1% - +1% -3.69ms - +4.85ms	-

_{tachometer-reporter-action v2 for Benchmarks}

packages/reactive-element/src/decorators/query-assigned-elements.ts

aomarks · 2021-12-02T21:48:05Z

packages/ts-transformers/src/internal/decorators/query-assigned-elements.ts

+      if (
+        propAssignment &&
+        ts.isPropertyAssignment(propAssignment) &&
+        ts.isStringLiteral(propAssignment.initializer)


Should probably throw if it's not a string literal, because it could also be a variable and we'd silently ignore that here.

Done thank you!

Actually just occured to me that selector could be used as-is whatever kind of node it is, since it just goes directly into the filter arguments, right?

And you could maybe also handle slot being an identifier or whatever too by generating `slot[name=${node}]` in the case that it's not a string?

packages/reactive-element/src/decorators/query-assigned-elements.ts

packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts

.changeset/famous-feet-kneel.md

packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts

packages/labs/ssr/src/lib/module-loader.ts

AndrewJakubowicz · 2021-12-03T01:44:02Z

Tests are passing. The only tests failing is something to do with Sauce not connecting:

ests: Error: 3 Dec 01:36:24 - Sauce Connect could not establish a connection.
tests: 3 Dec 01:36:24 - Please check your firewall and proxy settings.
tests: 3 Dec 01:36:24 - Goodbye.
tests:     at Socket.<anonymous> (/home/runner/work/lit/lit/packages/tests/node_modules/saucelabs/build/index.js:322:25)
tests:     at Socket.emit (events.js:400:28)
tests:     at Socket.emit (domain.js:475:12)
tests:     at addChunk (internal/streams/readable.js:293:12)
tests:     at readableAddChunk (internal/streams/readable.js:267:9)
tests:     at Socket.Readable.push (internal/streams/readable.js:206:10)
tests:     at Pipe.onStreamRead (internal/stream_base_commons.js:188:23)

I think this is unrelated to my changes.

aomarks

This is great! Awesome to do get the transformer code in too.

.changeset/famous-feet-kneel.md

packages/reactive-element/src/decorators/query-assigned-elements.ts

aomarks · 2021-12-04T02:17:43Z

packages/ts-transformers/src/internal/decorators/query-assigned-elements.ts

+      if (
+        propAssignment &&
+        ts.isPropertyAssignment(propAssignment) &&
+        ts.isStringLiteral(propAssignment.initializer)


Actually just occured to me that selector could be used as-is whatever kind of node it is, since it just goes directly into the filter arguments, right?

And you could maybe also handle slot being an identifier or whatever too by generating `slot[name=${node}]` in the case that it's not a string?

…signedElements Add references to the new internal decorator across the monorepo packages.

…gnedElements.

`slot` instead of `slotName`. Fix up copy pasted tests such that they are more modern, remove unused pieces. Update the transformer tests.

…ing found.

In this change made the transformer far more flexible. It no longer forces string literal arguments, and instead utilizes template strings to handle arbitrary expressions or identifiers.

packages/ts-transformers/src/internal/decorators/query-assigned-elements.ts

aomarks

💯

packages/reactive-element/src/decorators/query-assigned-elements.ts

packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts

Some documentation updates and a missed optional chaining.

google-cla bot added the cla: yes label Dec 1, 2021

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch from 11899cc to 759c823 Compare December 1, 2021 03:01

justinfagnani reviewed Dec 1, 2021

View reviewed changes

packages/reactive-element/src/decorators/query-assigned-elements.ts Outdated Show resolved Hide resolved

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch 4 times, most recently from 9b8224b to d5d4506 Compare December 2, 2021 17:45

AndrewJakubowicz changed the title ~~[Feature] Add queryAssignedElements decorator~~ [reactive-element] Add queryAssignedElements decorator Dec 2, 2021

AndrewJakubowicz mentioned this pull request Dec 2, 2021

[reactive-element] Update queryAssignedNodes to accept options bag as an alternative #2332

Closed

AndrewJakubowicz marked this pull request as ready for review December 2, 2021 19:27

aomarks reviewed Dec 2, 2021

View reviewed changes

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch 3 times, most recently from 575b2e7 to 51b8df7 Compare December 2, 2021 22:04

justinfagnani requested changes Dec 2, 2021

View reviewed changes

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch 3 times, most recently from 4097a4e to 07417c2 Compare December 2, 2021 22:40

justinfagnani reviewed Dec 2, 2021

View reviewed changes

.changeset/famous-feet-kneel.md Outdated Show resolved Hide resolved

packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts Show resolved Hide resolved

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch 2 times, most recently from 34fb99c to 6149cbf Compare December 3, 2021 00:55

justinfagnani reviewed Dec 3, 2021

View reviewed changes

packages/labs/ssr/src/lib/module-loader.ts Outdated Show resolved Hide resolved

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch 2 times, most recently from 367cc64 to 863ecc0 Compare December 3, 2021 01:27

AndrewJakubowicz requested review from justinfagnani and aomarks December 3, 2021 01:56

aomarks requested changes Dec 4, 2021

View reviewed changes

AndrewJakubowicz added 7 commits December 6, 2021 09:49

Add queryAssignedElements decorator behaving as a declarative slot.as…

02ac04b

…signedElements Add references to the new internal decorator across the monorepo packages.

Add QueryAssignedElementsVisitor to ts-transformers for new queryAssi…

3534ccd

…gnedElements.

Code review feedback.

5437e70

`slot` instead of `slotName`. Fix up copy pasted tests such that they are more modern, remove unused pieces. Update the transformer tests.

Add changeset

e114771

More code review feedback.

f5f5353

Unblock ssr test breakage caused by query-assigned-elements.js not be…

7a8f736

…ing found.

Address ts-transformers feedback and allow shorthand properties.

8413ed3

In this change made the transformer far more flexible. It no longer forces string literal arguments, and instead utilizes template strings to handle arbitrary expressions or identifiers.

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch from 1c3c46d to 8413ed3 Compare December 6, 2021 20:22

AndrewJakubowicz requested a review from aomarks December 6, 2021 20:24

aomarks reviewed Dec 6, 2021

View reviewed changes

packages/ts-transformers/src/internal/decorators/query-assigned-elements.ts Show resolved Hide resolved

Transformer code review feedback: inline string literals & update tests.

c3d0506

aomarks approved these changes Dec 6, 2021

View reviewed changes

justinfagnani requested changes Dec 6, 2021

View reviewed changes

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch from 4255c96 to ad439fc Compare December 7, 2021 01:18

justinfagnani added this to the Lit 2.1 milestone Dec 7, 2021

justinfagnani approved these changes Dec 7, 2021

View reviewed changes

packages/reactive-element/src/decorators/query-assigned-elements.ts Outdated Show resolved Hide resolved

packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts Show resolved Hide resolved

Add feedback from Justin.

b3dae4b

Some documentation updates and a missed optional chaining.

AndrewJakubowicz force-pushed the feat-query-assigned-elements branch from ad439fc to b3dae4b Compare December 7, 2021 05:02

AndrewJakubowicz mentioned this pull request Dec 7, 2021

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

Merged

AndrewJakubowicz merged commit 49ecf62 into main Dec 7, 2021

AndrewJakubowicz deleted the feat-query-assigned-elements branch December 7, 2021 18:18

This was referenced Jan 5, 2022

Version Packages #2375

Closed

Version Packages #2377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reactive-element] Add queryAssignedElements decorator #2327

[reactive-element] Add queryAssignedElements decorator #2327

AndrewJakubowicz commented Dec 1, 2021 •

edited

changeset-bot bot commented Dec 1, 2021 •

edited

github-actions bot commented Dec 1, 2021 •

edited

📊 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

aomarks Dec 2, 2021

AndrewJakubowicz Dec 2, 2021

aomarks Dec 4, 2021

AndrewJakubowicz commented Dec 3, 2021

aomarks left a comment

aomarks Dec 4, 2021

aomarks left a comment

[reactive-element] Add queryAssignedElements decorator #2327

[reactive-element] Add queryAssignedElements decorator #2327

Conversation

AndrewJakubowicz commented Dec 1, 2021 • edited

How

Testing

changeset-bot bot commented Dec 1, 2021 • edited

🦋 Changeset detected

github-actions bot commented Dec 1, 2021 • edited

📊 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

aomarks Dec 2, 2021

Choose a reason for hiding this comment

AndrewJakubowicz Dec 2, 2021

Choose a reason for hiding this comment

aomarks Dec 4, 2021

Choose a reason for hiding this comment

AndrewJakubowicz commented Dec 3, 2021

aomarks left a comment

Choose a reason for hiding this comment

aomarks Dec 4, 2021

Choose a reason for hiding this comment

aomarks left a comment

Choose a reason for hiding this comment

AndrewJakubowicz commented Dec 1, 2021 •

edited

changeset-bot bot commented Dec 1, 2021 •

edited

github-actions bot commented Dec 1, 2021 •

edited