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

[MaxArt2501]

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.

Might follow a PR for lit.dev repo to state the new behavior.

[changeset-bot]

⚠️ No Changeset found

Latest commit: f9bdd06

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[github-actions]

📊 Tachometer Benchmark Results

Summary

A summary of the benchmark results will show here once they finish.

Results

The full results of your benchmarks will show here once they finish.

_{tachometer-reporter-action v2 for Benchmarks}

[MaxArt2501]

I've removed the if (cache) statement as it should always be true at this point. Possibly a copy+paste mistake?

[justinfagnani]

good catch!

[justinfagnani]

A couple of quick comments, though I want to take discussion of the change in behavior to #4237

[justinfagnani]

good catch!

[rictic]

Consider add in our DEV_MODE only warning issuer code to this file:

let issueWarning: undefined | (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> = (global.litIssuedWarnings ??=
    new Set());

  // Issue a warning, if we haven't already.
  issueWarning = (code: string, warning: string) => {
    warning += ` See https://lit.dev/msg/${code} for more information.`;
    if (!issuedWarnings.has(warning)) {
      console.warn(warning);
      issuedWarnings.add(warning);
    }
  };
}

And issuing a warning in doQuery if DEV_MODE is true and !el.hasUpdated, letting the user know that their query field is being accessed before the element has updated.

[MaxArt2501]

Should the warning be for any access before the first update, whether the result is null or not, and whether the query is cached or not?
Should I create a code of my choice or there's a guideline for this? I was thinking of 'early-cached-query-access'.

[MaxArt2501]

Added the warning with a tentative code and message. Let me know if it's ok.

[justinfagnani]

One note: we need to update the output of the ts-transformers package so that the behavior of the emitted query getter matches this change.

[AndrewJakubowicz]

One note: we need to update the output of the ts-transformers package so that the behavior of the emitted query getter matches this change.

I just verified that this is not needed.
The idiomatic decorator transform already has this behavior.

The transform does the following:

@query('#mySpan', true)
span?: HTMLSpanElement

// after idiomatic transform
get span() {
  return this.__span ??= this.renderRoot?.querySelector('#mySpan') ?? null;
}

Because of the nullish coalescing operator, the idiomatic decorator transform won't cache the null value.

[MaxArt2501]

I'm not sure I need to do something to make this PR approved. I see unsuccessful checks - is theere something I should/can do?
Will it make it in time for this Tuesday's release?

[rictic]

Oh that's messed up, deleting the 3.0 branch closed this PR? In the past in similar cases it just changed the target branch of the PR

[AndrewJakubowicz]

Sorry that this PR closed. We're unable to reopen it on our end (probably because it's from a fork).

Would you be able to re-create this change, and base it off main branch (ideally using the email with the CLA)?

We had a discussion and decided this isn't blocking 3.0, and can be added as a minor patch to 3.0.

The rationale is that, in the rare case that this breaks someone, it probably should already be an error. The error case would hopefully also be noticed, so a cached null value isn't depended on.

[MaxArt2501]

Sure, no problem! I'll do that as soon as possible.

Thank you