[lit/reactive-element] Make @query not cache null results before firs…

…t render (#4282)

Fixes: #4237

Co-authored-by: Andrew Jakubowicz <ajakubowicz@google.com>
Co-authored-by: Augustine Kim <ajk830@gmail.com>

.changeset/quick-moons-hear.md

@@ -0,0 +1,7 @@
+---
+'@lit/reactive-element': patch
+'lit-element': patch
+'lit': patch
+---
+
+Fix a bug where accessing a `@query` decorated field with the `cache` flag set before the first update would result in `null` being cached permanently. `null` will no longer be cached before the first update and in `DEV_MODE` now raises a warning.

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

@@ -15,6 +15,26 @@ import {desc, type Interface} from './base.js';
 
 const DEV_MODE = true;
 
+let issueWarning: (code: string, warning: string) => void;
+
+if (DEV_MODE) {
+  // Ensure warnings are issued only 1x, even if multiple versions of Lit
+  // are loaded.
+  const issuedWarnings: Set<string | undefined> =
+    (globalThis.litIssuedWarnings ??= new Set());
+
+  // Issue a warning, if we haven't already.
+  issueWarning = (code: string, warning: string) => {
+    warning += code
+      ? ` See https://lit.dev/msg/${code} for more information.`
+      : '';
+    if (!issuedWarnings.has(warning)) {
+      console.warn(warning);
+      issuedWarnings.add(warning);
+    }
+  };
+}
+
 export type QueryDecorator = {
   // legacy
   (
@@ -64,10 +84,25 @@ export function query(selector: string, cache?: boolean): QueryDecorator {
     descriptor?: PropertyDescriptor
   ) => {
     const doQuery = (el: Interface<ReactiveElement>): V => {
+      const result = (el.renderRoot?.querySelector(selector) ?? null) as V;
+      if (DEV_MODE && result === null && cache && !el.hasUpdated) {
+        const name =
+          typeof nameOrContext === 'object'
+            ? nameOrContext.name
+            : nameOrContext;
+        issueWarning(
+          '',
+          `@query'd field ${JSON.stringify(String(name))} with the 'cache' ` +
+            `flag set for selector '${selector}' has been accessed before ` +
+            `the first update and returned null. This is expected if the ` +
+            `renderRoot tree has not been provided beforehand (e.g. via ` +
+            `Declarative Shadow DOM). Therefore the value hasn't been cached.`
+        );
+      }
       // TODO: if we want to allow users to assert that the query will never
       // return null, we need a new option and to throw here if the result
       // is null.
-      return (el.renderRoot?.querySelector(selector) ?? null) as V;
+      return result;
     };
     if (cache) {
       // Accessors to wrap from either:
@@ -98,15 +133,14 @@ export function query(selector: string, cache?: boolean): QueryDecorator {
             })();
       return desc(protoOrTarget, nameOrContext, {
         get(this: ReactiveElement): V {
-          if (cache) {
-            let result: V = get!.call(this);
-            if (result === undefined) {
-              result = doQuery(this);
+          let result: V = get!.call(this);
+          if (result === undefined) {
+            result = doQuery(this);
+            if (result !== null || this.hasUpdated) {
               set!.call(this, result);
             }
-            return result;
           }
-          return doQuery(this);
+          return result;
         },
       });
     } else {

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

@@ -96,11 +96,27 @@ import {assert} from '@esm-bundle/chai';
     assert.notEqual(el.span, el.renderRoot.querySelector('span'));
   });
 
-  test('returns cached value when accessed before first update', async () => {
+  test('does not cache null values when accessed before first update', async () => {
     const notYetUpdatedEl = new C();
     assert.equal(notYetUpdatedEl.divCached, null);
+
+    if (globalThis.litIssuedWarnings != null) {
+      assert(
+        [...globalThis.litIssuedWarnings].find((w) =>
+          /@query'd field "divCached" with the 'cache' flag set for selector '#blah' has been accessed before the first update and returned null\. This is expected if the renderRoot tree has not been provided beforehand \(e\.g\. via Declarative Shadow DOM\)\. Therefore the value hasn't been cached\./.test(
+            w ?? ''
+          )
+        ),
+        `Expected warning to be issued. Warnings found: ${JSON.stringify(
+          [...globalThis.litIssuedWarnings],
+          null,
+          2
+        )}`
+      );
+    }
+
     container.appendChild(notYetUpdatedEl);
     await notYetUpdatedEl.updateComplete;
-    assert.equal(notYetUpdatedEl.divCached, null);
+    assert.instanceOf(notYetUpdatedEl.divCached, HTMLDivElement);
   });
 });

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

@@ -90,12 +90,28 @@ import {assert} from '@esm-bundle/chai';
     assert.notEqual(el.span, el.renderRoot.querySelector('span'));
   });
 
-  test('returns cached value when accessed before first update', async () => {
+  test('does not cache null values when accessed before first update', async () => {
     const notYetUpdatedEl = new C();
     assert.equal(notYetUpdatedEl.divCached, null);
+
+    if (globalThis.litIssuedWarnings != null) {
+      assert(
+        [...globalThis.litIssuedWarnings].find((w) =>
+          /@query'd field "divCached" with the 'cache' flag set for selector '#blah' has been accessed before the first update and returned null\. This is expected if the renderRoot tree has not been provided beforehand \(e\.g\. via Declarative Shadow DOM\)\. Therefore the value hasn't been cached\./.test(
+            w ?? ''
+          )
+        ),
+        `Expected warning to be issued. Warnings found: ${JSON.stringify(
+          [...globalThis.litIssuedWarnings],
+          null,
+          2
+        )}`
+      );
+    }
+
     container.appendChild(notYetUpdatedEl);
     await notYetUpdatedEl.updateComplete;
-    assert.equal(notYetUpdatedEl.divCached, null);
+    assert.instanceOf(notYetUpdatedEl.divCached, HTMLDivElement);
   });
 
   test('works with an old and busted Reflect.decorate', async () => {