@query and @queryAll return null or [] before first update #2065

sorvell · 2021-08-19T22:57:51Z

Fixes #2062. To match Lit1 behavior, the @query decorator returns null (rather than undefined) if a decorated property is accessed before first update. Note, when the cache option is used, the first result is always cached and it must be either null or the found element. Likewise, a @queryAll decorated property returns [] rather than undefined.

Fixes #2062. To match Lit1 behavior, the `@query` decorator returns `null` (rather than `undefined`) if a decorated property is accessed before first update. Likewise, a `@queryAll` decorated property returns `[]` rather than `undefined`.

changeset-bot · 2021-08-19T22:57:53Z

🦋 Changeset detected

Latest commit: 12d4c74

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-08-19T22:58:14Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: slower ❌ 8% - 29% (3.03ms - 10.25ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -6% - +1% (-5.30ms - +0.67ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -0% - +14% (-0.15ms - +4.90ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +5% (-0.33ms - +0.57ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +3% (-0.59ms - +1.61ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +3% (-2.06ms - +1.63ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -4% - +2% (-30.57ms - +14.54ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -1% - +12% (-0.47ms - +11.70ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -0% - +4% (-0.72ms - +14.20ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-2.37ms - +2.64ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +2% (-21.68ms - +15.06ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -3% - +4% (-24.85ms - +26.83ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +4% (-17.68ms - +27.27ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1712
Commit: cc93018

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	82.35ms - 86.53ms	-	unsure 🔍 -6% - +1% -5.30ms - +0.67ms	faster ✔ 20% - 26% 21.42ms - 28.62ms
tip-of-tree tip-of-tree	84.62ms - 88.88ms	unsure 🔍 -1% - +6% -0.67ms - +5.30ms	-	faster ✔ 18% - 24% 19.08ms - 26.33ms
previous-release previous-release	106.52ms - 112.39ms	slower ❌ 25% - 34% 21.42ms - 28.62ms	slower ❌ 22% - 31% 19.08ms - 26.33ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	668.18ms - 698.71ms	-	unsure 🔍 -4% - +2% -30.57ms - +14.54ms	faster ✔ 8% - 14% 60.41ms - 107.73ms
tip-of-tree tip-of-tree	674.85ms - 708.07ms	unsure 🔍 -2% - +4% -14.54ms - +30.57ms	-	faster ✔ 7% - 13% 51.50ms - 100.61ms
previous-release previous-release	749.44ms - 785.59ms	slower ❌ 9% - 16% 60.41ms - 107.73ms	slower ❌ 7% - 15% 51.50ms - 100.61ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	707.82ms - 745.90ms	-	unsure 🔍 -3% - +4% -24.85ms - +26.83ms	faster ✔ 1% - 8% 7.95ms - 60.12ms
tip-of-tree tip-of-tree	708.40ms - 743.34ms	unsure 🔍 -4% - +3% -26.83ms - +24.85ms	-	faster ✔ 1% - 8% 10.06ms - 59.98ms
previous-release previous-release	743.06ms - 778.72ms	slower ❌ 1% - 8% 7.95ms - 60.12ms	slower ❌ 1% - 8% 10.06ms - 59.98ms	-

lit-html-kitchen-sink

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1712
Commit: cc93018

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	36.17ms - 40.83ms	-	unsure 🔍 -0% - +14% -0.15ms - +4.90ms	faster ✔ 23% - 36% 11.40ms - 20.72ms
tip-of-tree tip-of-tree	35.15ms - 37.09ms	unsure 🔍 -12% - +0% -4.90ms - +0.15ms	-	faster ✔ 29% - 39% 14.28ms - 22.58ms
previous-release previous-release	50.52ms - 58.59ms	slower ❌ 28% - 55% 11.40ms - 20.72ms	slower ❌ 39% - 63% 14.28ms - 22.58ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	100.06ms - 109.70ms	-	unsure 🔍 -1% - +12% -0.47ms - +11.70ms	unsure 🔍 -8% - +4% -8.14ms - +4.35ms
tip-of-tree tip-of-tree	95.55ms - 102.97ms	unsure 🔍 -11% - +0% -11.70ms - +0.47ms	-	faster ✔ 2% - 12% 2.08ms - 12.94ms
previous-release previous-release	102.80ms - 110.74ms	unsure 🔍 -4% - +8% -4.35ms - +8.14ms	slower ❌ 2% - 13% 2.08ms - 12.94ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	40.05ms - 45.61ms	-	slower ❌ 8% - 29% 3.03ms - 10.25ms	slower ❌ 20% - 43% 6.94ms - 13.55ms
tip-of-tree tip-of-tree	33.88ms - 38.50ms	faster ✔ 8% - 23% 3.03ms - 10.25ms	-	slower ❌ 2% - 20% 0.68ms - 6.53ms
previous-release previous-release	30.79ms - 34.38ms	faster ✔ 17% - 30% 6.94ms - 13.55ms	faster ✔ 2% - 18% 0.68ms - 6.53ms	-

lit-html-repeat

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1712
Commit: cc93018

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.17ms - 11.92ms	-	unsure 🔍 -3% - +5% -0.33ms - +0.57ms	faster ✔ 8% - 14% 0.99ms - 1.86ms
tip-of-tree tip-of-tree	11.17ms - 11.67ms	unsure 🔍 -5% - +3% -0.57ms - +0.33ms	-	faster ✔ 9% - 14% 1.21ms - 1.89ms
previous-release previous-release	12.75ms - 13.20ms	slower ❌ 8% - 16% 0.99ms - 1.86ms	slower ❌ 10% - 17% 1.21ms - 1.89ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	355.61ms - 368.84ms	-	unsure 🔍 -0% - +4% -0.72ms - +14.20ms	faster ✔ 25% - 28% 121.48ms - 138.60ms
tip-of-tree tip-of-tree	352.05ms - 358.93ms	unsure 🔍 -4% - +0% -14.20ms - +0.72ms	-	faster ✔ 27% - 29% 130.35ms - 143.21ms
previous-release previous-release	486.84ms - 497.70ms	slower ❌ 33% - 39% 121.48ms - 138.60ms	slower ❌ 36% - 41% 130.35ms - 143.21ms	-

lit-html-template-heavy

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1712
Commit: cc93018

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	57.34ms - 59.00ms	-	unsure 🔍 -1% - +3% -0.59ms - +1.61ms	faster ✔ 16% - 20% 11.23ms - 14.06ms
tip-of-tree tip-of-tree	56.94ms - 58.39ms	unsure 🔍 -3% - +1% -1.61ms - +0.59ms	-	faster ✔ 17% - 20% 11.80ms - 14.51ms
previous-release previous-release	69.67ms - 71.96ms	slower ❌ 19% - 24% 11.23ms - 14.06ms	slower ❌ 20% - 25% 11.80ms - 14.51ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	114.23ms - 117.40ms	-	unsure 🔍 -2% - +2% -2.37ms - +2.64ms	faster ✔ 10% - 13% 12.63ms - 17.40ms
tip-of-tree tip-of-tree	113.74ms - 117.62ms	unsure 🔍 -2% - +2% -2.64ms - +2.37ms	-	faster ✔ 10% - 13% 12.51ms - 17.79ms
previous-release previous-release	129.05ms - 132.61ms	slower ❌ 11% - 15% 12.63ms - 17.40ms	slower ❌ 11% - 16% 12.51ms - 17.79ms	-

reactive-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1712
Commit: cc93018

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	58.02ms - 60.48ms	-	unsure 🔍 -3% - +3% -2.06ms - +1.63ms	unsure 🔍 -3% - +3% -1.93ms - +1.69ms
tip-of-tree tip-of-tree	58.08ms - 60.84ms	unsure 🔍 -3% - +3% -1.63ms - +2.06ms	-	unsure 🔍 -3% - +3% -1.82ms - +2.00ms
previous-release previous-release	58.05ms - 60.69ms	unsure 🔍 -3% - +3% -1.69ms - +1.93ms	unsure 🔍 -3% - +3% -2.00ms - +1.82ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	660.94ms - 685.18ms	-	unsure 🔍 -3% - +2% -21.68ms - +15.06ms	unsure 🔍 -2% - +3% -13.17ms - +20.83ms
tip-of-tree tip-of-tree	662.56ms - 690.17ms	unsure 🔍 -2% - +3% -15.06ms - +21.68ms	-	unsure 🔍 -2% - +4% -11.10ms - +25.38ms
previous-release previous-release	657.31ms - 681.15ms	unsure 🔍 -3% - +2% -20.83ms - +13.17ms	unsure 🔍 -4% - +2% -25.38ms - +11.10ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	729.56ms - 761.21ms	-	unsure 🔍 -2% - +4% -17.68ms - +27.27ms	unsure 🔍 -2% - +4% -11.97ms - +29.23ms
tip-of-tree tip-of-tree	724.63ms - 756.55ms	unsure 🔍 -4% - +2% -27.27ms - +17.68ms	-	unsure 🔍 -2% - +3% -16.87ms - +24.54ms
previous-release previous-release	723.57ms - 749.94ms	unsure 🔍 -4% - +2% -29.23ms - +11.97ms	unsure 🔍 -3% - +2% -24.54ms - +16.87ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2021-08-19T23:50:09Z

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

+          return (
+            (this as unknown as {[key: string]: Element | null})[
+              key as string
+            ] ?? null


should we initialize the cache property to null so that we avoid later hidden class changes and then check for null instead of undefined above?

There's no real place to do that right? You can't do it at decorator time since it's an own property on the instance. And there's not a good initializer spot to do it before the accessor is run the first time ... 🤷 .

This wasn't quite correct. The intended behavior is to always cache the first result and that the result must be either null or the found element.

Always cache the first result, but ensure it's at least null.

kevinpschaaf

LGTM

aomarks · 2021-08-23T03:42:05Z

This is a bit of a shame in terms of decorator -> vanilla JS documentation/transformer, since it requires adding ` ?? null` or ` ?? []` everyplace we transform `@query` and `@queryAll`, which doesn't read as nicely, and I don't think a person would usually think to write that otherwise. It's also a little less intuitive behaviorally since you'd expect it to match the types of the built-in `query` and `queryAll`.

~~Any chance we could keep this as a breaking change, or do we think the cost is too high?~~

Edit: Nevermind, I misunderstood what was going on here. New change makes sense.

@query

…update (#2095) Aligns transformer behavior to #2065 and #1980 by making the @query, @QueryAll, and @queryAssignedNodes decorator return null / [] / [] respectively before first update, instead of undefined. Also includes a fix where we were removing no-binding imports that already existed in the source. We only want to remove imports that we actually modified.

sorvell requested a review from kevinpschaaf August 19, 2021 22:57

sorvell requested a review from justinfagnani as a code owner August 19, 2021 22:57

google-cla bot added the cla: yes label Aug 19, 2021

Add changeset

5a08a07

justinfagnani reviewed Aug 19, 2021

View reviewed changes

sorvell added this to the Lit RC.next milestone Aug 20, 2021

Actually match Lit1 behavior for query + cache

12d4c74

Always cache the first result, but ensure it's at least null.

kevinpschaaf approved these changes Aug 20, 2021

View reviewed changes

justinfagnani approved these changes Aug 20, 2021

View reviewed changes

sorvell merged commit 8b6e241 into main Aug 20, 2021

sorvell deleted the query branch August 20, 2021 16:59

This was referenced Aug 26, 2021

@queryAssignedNodes: return [] before first update #2094

Closed

[ts-transformers] Return null or [] on query decorators before first update #2095

Merged

@query and @queryAll return null or [] before first update #2065

@query and @queryAll return null or [] before first update #2065

Uh oh!

Conversation

sorvell commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Aug 19, 2021 • 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 19, 2021 • 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!

justinfagnani Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

sorvell Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

aomarks commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sorvell commented Aug 19, 2021 •

edited

Loading

changeset-bot bot commented Aug 19, 2021 •

edited

Loading

github-actions bot commented Aug 19, 2021 •

edited

Loading

aomarks commented Aug 23, 2021 •

edited

Loading