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

.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,24 @@ 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))} 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).`
+        );
+      }
       // 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 +132,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" for selector '#blah' 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\)\./.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" for selector '#blah' 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\)\./.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 () => {