Make @query not cache null results before first render ( #4237)#4259
Make @query not cache null results before first render ( #4237)#4259MaxArt2501 wants to merge 5 commits intolit:3.0from
Conversation
|
|
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. |
📊 Tachometer Benchmark ResultsSummaryA summary of the benchmark results will show here once they finish. ResultsThe full results of your benchmarks will show here once they finish. |
| let result: V = get!.call(this); | ||
| if (result === undefined) { | ||
| result = doQuery(this); | ||
| let result: V = get!.call(this); |
There was a problem hiding this comment.
I've removed the if (cache) statement as it should always be true at this point. Possibly a copy+paste mistake?
justinfagnani
left a comment
There was a problem hiding this comment.
A couple of quick comments, though I want to take discussion of the change in behavior to #4237
| let result: V = get!.call(this); | ||
| if (result === undefined) { | ||
| result = doQuery(this); | ||
| let result: V = get!.call(this); |
| @@ -98,15 +98,14 @@ export function query(selector: string, cache?: boolean): QueryDecorator { | |||
| })(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'.
There was a problem hiding this comment.
Added the warning with a tentative code and message. Let me know if it's ok.
|
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 transform does the following: Because of the nullish coalescing operator, the idiomatic decorator transform won't cache the |
|
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? |
|
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 |
|
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 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 |
|
Sure, no problem! I'll do that as soon as possible. Thank you |
This change makes the
@querydecorator not cache the result if it'snulland 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.