Make @query not cache null results before first render (#4237) by MaxArt2501 · Pull Request #4282 · lit/lit

MaxArt2501 · 2023-10-10T18:57:07Z

Do-over of #4259 after being closed when branch 3.0 has been closed. This was the description:

This change makes the @query decorator not cache the result if it's null and before the first render.
The result might be not null in case of a Declarative Shadow DOM.

changeset-bot · 2023-10-10T18:57:11Z

🦋 Changeset detected

Latest commit: 8c62985

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

This PR includes changesets to release 3 packages

Name	Type
@lit/reactive-element	Patch
lit-element	Patch
lit	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 · 2023-10-10T18:57:29Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +9% (-0.10ms - +0.98ms)
^{this-change vs tip-of-tree}

render

this-change: 49.08ms - 52.04ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +5% (-0.12ms - +0.99ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.78ms - +0.86ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +5% (-0.26ms - +1.70ms)
^{this-change vs tip-of-tree}

update

this-change: 535.87ms - 549.28ms
this-change, tip-of-tree, previous-release: slower ❌ 1% - 13% (0.59ms - 5.03ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +4% (-2.76ms - +2.94ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +3% (-8.61ms - +15.56ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 527.72ms - 546.36ms
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-8.47ms - +10.85ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 118.0.5993.117
Sample size: 50
Built by: Benchmarks #5832
Commit: 8c62985

render

Version	Avg time	vs
	49.08ms - 52.04ms	-

update

Version	Avg time	vs
	535.87ms - 549.28ms	-

update-reflect

Version	Avg time	vs
	527.72ms - 546.36ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 118.0.5993.117
Sample size: 30
Built by: Benchmarks #5832
Commit: 8c62985

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.30ms - 19.23ms	-	unsure 🔍 -1% - +5% -0.12ms - +0.99ms	unsure 🔍 -3% - +4% -0.56ms - +0.70ms
tip-of-tree tip-of-tree	18.03ms - 18.64ms	unsure 🔍 -5% - +1% -0.99ms - +0.12ms	-	unsure 🔍 -5% - +1% -0.89ms - +0.16ms
previous-release previous-release	18.27ms - 19.13ms	unsure 🔍 -4% - +3% -0.70ms - +0.56ms	unsure 🔍 -1% - +5% -0.16ms - +0.89ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	40.14ms - 43.97ms	-	slower ❌ 1% - 13% 0.59ms - 5.03ms	unsure 🔍 -8% - +5% -3.35ms - +2.11ms
tip-of-tree tip-of-tree	38.12ms - 40.36ms	faster ✔ 2% - 12% 0.59ms - 5.03ms	-	faster ✔ 3% - 13% 1.18ms - 5.69ms
previous-release previous-release	40.72ms - 44.63ms	unsure 🔍 -5% - +8% -2.11ms - +3.35ms	slower ❌ 3% - 15% 1.18ms - 5.69ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.41ms - 12.15ms	-	unsure 🔍 -1% - +9% -0.10ms - +0.98ms	unsure 🔍 -5% - +4% -0.62ms - +0.48ms
tip-of-tree tip-of-tree	10.94ms - 11.74ms	unsure 🔍 -8% - +1% -0.98ms - +0.10ms	-	unsure 🔍 -9% - +0% -1.08ms - +0.06ms
previous-release previous-release	11.44ms - 12.26ms	unsure 🔍 -4% - +5% -0.48ms - +0.62ms	unsure 🔍 -1% - +10% -0.06ms - +1.08ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 118.0.5993.117
Sample size: 50
Built by: Benchmarks #5832
Commit: 8c62985

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	34.61ms - 35.68ms	-	unsure 🔍 -2% - +2% -0.78ms - +0.86ms	unsure 🔍 -2% - +3% -0.62ms - +1.17ms
tip-of-tree tip-of-tree	34.48ms - 35.73ms	unsure 🔍 -2% - +2% -0.86ms - +0.78ms	-	unsure 🔍 -2% - +3% -0.71ms - +1.19ms
previous-release previous-release	34.15ms - 35.58ms	unsure 🔍 -3% - +2% -1.17ms - +0.62ms	unsure 🔍 -3% - +2% -1.19ms - +0.71ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	73.22ms - 77.01ms	-	unsure 🔍 -4% - +4% -2.76ms - +2.94ms	unsure 🔍 -4% - +4% -2.83ms - +2.92ms
tip-of-tree tip-of-tree	72.90ms - 77.15ms	unsure 🔍 -4% - +4% -2.94ms - +2.76ms	-	unsure 🔍 -4% - +4% -3.08ms - +2.99ms
previous-release previous-release	72.90ms - 77.23ms	unsure 🔍 -4% - +4% -2.92ms - +2.83ms	unsure 🔍 -4% - +4% -2.99ms - +3.08ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 118.0.5993.117
Sample size: 50
Built by: Benchmarks #5832
Commit: 8c62985

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.11ms - 34.87ms	-	unsure 🔍 -1% - +5% -0.26ms - +1.70ms	unsure 🔍 -1% - +5% -0.40ms - +1.55ms
tip-of-tree tip-of-tree	32.85ms - 33.69ms	unsure 🔍 -5% - +1% -1.70ms - +0.26ms	-	unsure 🔍 -2% - +1% -0.73ms - +0.44ms
previous-release previous-release	33.00ms - 33.83ms	unsure 🔍 -5% - +1% -1.55ms - +0.40ms	unsure 🔍 -1% - +2% -0.44ms - +0.73ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	528.77ms - 547.88ms	-	unsure 🔍 -2% - +3% -8.61ms - +15.56ms	unsure 🔍 -2% - +3% -8.11ms - +14.82ms
tip-of-tree tip-of-tree	527.46ms - 542.24ms	unsure 🔍 -3% - +2% -15.56ms - +8.61ms	-	unsure 🔍 -2% - +2% -9.86ms - +9.62ms
previous-release previous-release	528.63ms - 541.31ms	unsure 🔍 -3% - +1% -14.82ms - +8.11ms	unsure 🔍 -2% - +2% -9.62ms - +9.86ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	530.05ms - 544.56ms	-	unsure 🔍 -2% - +2% -8.47ms - +10.85ms	unsure 🔍 -2% - +2% -9.63ms - +9.54ms
tip-of-tree tip-of-tree	529.73ms - 542.49ms	unsure 🔍 -2% - +2% -10.85ms - +8.47ms	-	unsure 🔍 -2% - +1% -10.18ms - +7.70ms
previous-release previous-release	531.09ms - 543.61ms	unsure 🔍 -2% - +2% -9.54ms - +9.63ms	unsure 🔍 -1% - +2% -7.70ms - +10.18ms	-

_{tachometer-reporter-action v2 for Benchmarks}

AndrewJakubowicz · 2023-11-02T21:55:02Z

Sorry for the slow response here. Thank you for re-creating this PR. I'm manually importing and testing this PR against our internal test suite to see if there is any test failures that need to be handled.

Will report back tomorrow with results.

Edit for internal ref: cl/579048151

AndrewJakubowicz

This is looking great, thank you for working on this. I've tested internally and it looks like this is a safe change!

I've left some small changes. Would you also be able to add a Changeset entry for this PR?
Thank you!

packages/reactive-element/src/test/decorators-modern/query_test.ts

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

MaxArt2501 · 2023-11-04T19:03:55Z

@AndrewJakubowicz Will do! Thank you for your review 👍

MaxArt2501 · 2023-11-05T09:46:36Z

@AndrewJakubowicz I added a "patch" changeset for @lit/reactive-element. LMK if you need me to change it.

.changeset/quick-moons-hear.md

Co-authored-by: Andrew Jakubowicz <spyr1014@gmail.com>

AndrewJakubowicz

Thank you!

AndrewJakubowicz · 2023-11-06T23:29:09Z

Ah, I just noticed there is a test failure being reported with Task. The test is Tasks can depend on host rendered DOM, and it's failing because the Task is depending on a @query decorated field. This means it accesses that field before the first update which triggers the render.

I think the warning should be scoped to only cache: true cases, and that'll solve the test failure.

AndrewJakubowicz · 2023-11-06T23:30:42Z

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

    descriptor?: PropertyDescriptor
  ) => {
    const doQuery = (el: Interface<ReactiveElement>): V => {
+      if (DEV_MODE && !el.hasUpdated) {


To resolve the test failure, and to be more specific for the caching null case, I propose adding an && cache here.

Interesting. The warning itself is generic that the getter was called before first update, and not specific to the cache option.

I wonder if the failing task test should be updated instead to exclude this warning instead of opting out this warning. (also interesting to see that the task test does highlight one way the query decorated property might get accessed before first update).

I could also be convinced to just only warn when cache is true too. It's more impactful to raise attention if null is getting cached where as the warning for none-caching scenarios might just be excess noise.

Given that a test has shown a valid reason to access a @query decorated field before the first update (in a Task dependency array). I wouldn't want to silence the warning in only that test case, because we may end up having users that get the warning via a valid usage of Task depending on a @query field.

I think the warning makes sense if there is something bad going on, but the fix to cache behavior resolves the worst possible bugs. I propose removing the warnings from this PR.

I agree, it seems fine to not warn when there's a legit case to encounter it. @rictic originally suggested in the issue and PR to add a dev mode warning, what do you think about removing it?

There are some combinations I've considered for the warning. We could issue it if cache is true, but also when the query result is actually null. Would that be acceptable?

Caching null sounds like an unintended behavior so warning in that case sounds reasonable to me.

Yes please. Making the warning specific to: cache: true and result is null, is a perfect time for a warning. It's much more specific to the issue you are fixing.
Thanks!

.changeset/quick-moons-hear.md

Co-authored-by: Augustine Kim <ajk830@gmail.com>

github-actions · 2023-11-07T15:37:51Z

The size of lit-html.js and lit-core.min.js are as expected.

lit#4237)

AndrewJakubowicz · 2023-11-09T02:55:36Z

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

+          `@query'd field ${JSON.stringify(String(name))} for selector ` +
+            `'${selector}' has been accessed before the first update. This ` +
+            `yields a certain null result if the renderRoot tree has not ` +
+            `been provided beforehand (e.g. via Declarative Shadow DOM).`


Thanks for bearing with these changed, I think the last change required is to update the warning to relate to the situation that was handled. E.g., You are preventing the caching of a null value.

I was just wondering if the message should closely reflect the conditions for issuing the warning 👍
I've kept a brief explanation of the cause since there isn't a detailed explanation in the documentation yet. I think it could be removed later to keep the message concise.

AndrewJakubowicz

Nice work!

AndrewJakubowicz

Nit: Spelling issue in yieled. I've added the three suggestions to replace it. I changed it to returned because yielded has generator connotations due to the yield keyword.

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

packages/reactive-element/src/test/decorators-modern/query_test.ts

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

MaxArt2501 · 2023-11-10T08:27:41Z

Thank you, folks, for all the support!

Make @query not cache null results before first render (lit#4237)

9113e45

MaxArt2501 requested a review from kevinpschaaf as a code owner October 10, 2023 18:57

Merge branch 'main' into main

553dae2

AndrewJakubowicz self-requested a review November 2, 2023 01:12

AndrewJakubowicz reviewed Nov 3, 2023

View reviewed changes

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

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

AndrewJakubowicz self-assigned this Nov 3, 2023

MaxArt2501 added 3 commits November 5, 2023 10:35

Add test assertions for issued warnings (lit#4237)

2164be5

Remove issued warning code (lit#4237)

6bab65d

Add changeset (lit#4237)

4cacec2

AndrewJakubowicz requested review from augustjk and rictic November 6, 2023 20:52

Merge branch 'main' into main

ac4f8d4

AndrewJakubowicz reviewed Nov 6, 2023

View reviewed changes

.changeset/quick-moons-hear.md Outdated Show resolved Hide resolved

Update .changeset/quick-moons-hear.md

756902e

Co-authored-by: Andrew Jakubowicz <spyr1014@gmail.com>

AndrewJakubowicz approved these changes Nov 6, 2023

View reviewed changes

AndrewJakubowicz reviewed Nov 6, 2023

View reviewed changes

AndrewJakubowicz self-requested a review November 7, 2023 00:23

augustjk reviewed Nov 7, 2023

View reviewed changes

.changeset/quick-moons-hear.md Show resolved Hide resolved

Update .changeset/quick-moons-hear.md

a370b9f

Co-authored-by: Augustine Kim <ajk830@gmail.com>

MaxArt2501 and others added 2 commits November 8, 2023 14:26

Limit the @query early access warning to cached cases and null results (

fa6b488

lit#4237)

Merge branch 'main' into main

468fc2b

AndrewJakubowicz reviewed Nov 9, 2023

View reviewed changes

Update issued warning message to reflect check conditions (lit#4237)

ccd4b71

augustjk approved these changes Nov 9, 2023

View reviewed changes

AndrewJakubowicz approved these changes Nov 9, 2023

View reviewed changes

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

packages/reactive-element/src/test/decorators-modern/query_test.ts Outdated Show resolved Hide resolved

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

Apply suggestions from code review

8c62985

AndrewJakubowicz enabled auto-merge (squash) November 9, 2023 22:45

AndrewJakubowicz merged commit c7922a0 into lit:main Nov 9, 2023

This was referenced Nov 14, 2023

Version Packages #4397

Closed

Version Packages #4399

Merged

Conversation

MaxArt2501 commented Oct 10, 2023

Uh oh!

changeset-bot bot commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Oct 10, 2023 • 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

update-reflect

Uh oh!

AndrewJakubowicz commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewJakubowicz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MaxArt2501 commented Nov 4, 2023

Uh oh!

MaxArt2501 commented Nov 5, 2023

Uh oh!

Uh oh!

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJakubowicz commented Nov 6, 2023

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!

Uh oh!

github-actions bot commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxArt2501 commented Nov 10, 2023

changeset-bot bot commented Oct 10, 2023 •

edited

Loading

github-actions bot commented Oct 10, 2023 •

edited

Loading

AndrewJakubowicz commented Nov 2, 2023 •

edited

Loading

AndrewJakubowicz left a comment •

edited

Loading