Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make @query not cache null results before first render (#4237) #4282
Changes from 6 commits
9113e45
553dae2
2164be5
6bab65d
4cacec2
ac4f8d4
756902e
a370b9f
fa6b488
468fc2b
ccd4b71
8c62985
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 ifnull
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 ofTask
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 actuallynull
. 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 isnull
, is a perfect time for a warning. It's much more specific to the issue you are fixing.Thanks!
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.