New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make @query not cache null results before first render (#4237) #4282
Conversation
🦋 Changeset detectedLatest commit: 8c62985 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@AndrewJakubowicz Will do! Thank you for your review 👍 |
@AndrewJakubowicz I added a "patch" changeset for @lit/reactive-element. LMK if you need me to change it. |
Co-authored-by: Andrew Jakubowicz <spyr1014@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Ah, I just noticed there is a test failure being reported with I think the warning should be scoped to only |
@@ -64,6 +84,19 @@ export function query(selector: string, cache?: boolean): QueryDecorator { | |||
descriptor?: PropertyDescriptor | |||
) => { | |||
const doQuery = (el: Interface<ReactiveElement>): V => { | |||
if (DEV_MODE && !el.hasUpdated) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To resolve the test failure, and to be more specific for the caching null
case, I propose adding an && cache
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caching null sounds like an unintended behavior so warning in that case sounds reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Co-authored-by: Augustine Kim <ajk830@gmail.com>
The size of lit-html.js and lit-core.min.js are as expected. |
`@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).` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/test/decorators-modern/query_test.ts
Outdated
Show resolved
Hide resolved
Thank you, folks, for all the support! |
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.