Skip to content
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

computed var default to cache=True #4194

Merged
merged 6 commits into from
Jan 16, 2025
Merged

Conversation

Lendemor
Copy link
Collaborator

Set the default value of cache to True for @rx.var

Lead to better performance "out of the box" since any computed var will be cached, unless explicitly disabled.

Potentially breaking some apps behaviour, so we are considering delaying this PR to 0.7.0.

adhami3310
adhami3310 previously approved these changes Oct 21, 2024
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we actually make the signature cache: bool = None

then in ComputedVar.__init__, we can check if cache is None and throw a deprecation warning, keeping the default as False until 0.7.0; then we remove the warning and replace the None default with True.

If users want to get rid of the warning, then they will need an explicit value for the cache parameter.

The main downside with this approach is that @rx.var decoration (with no parentheses) will raise the deprecation warning until 0.7.0 is released... maybe that's not ideal

@masenf masenf marked this pull request as draft October 22, 2024 01:02
@masenf
Copy link
Collaborator

masenf commented Oct 22, 2024

marking as draft to avoid accidentally merging a behavior-changing PR

@Lendemor
Copy link
Collaborator Author

Lendemor commented Dec 9, 2024

can we actually make the signature cache: bool = None

then in ComputedVar.__init__, we can check if cache is None and throw a deprecation warning, keeping the default as False until 0.7.0; then we remove the warning and replace the None default with True.

If users want to get rid of the warning, then they will need an explicit value for the cache parameter.

The main downside with this approach is that @rx.var decoration (with no parentheses) will raise the deprecation warning until 0.7.0 is released... maybe that's not ideal

I'm updating this PR in preparation for 0.7.0, do you still want any of those change to it?

@masenf
Copy link
Collaborator

masenf commented Dec 10, 2024

@Lendemor if we haven't been warning users that this behavior will change in 0.7.0, i think we should merge a patch for our next release that does as i suggested back in october and print a warning when the user does not provide an explicit boolean for the cache parameter. We need to warn users that this change is coming.

@Lendemor Lendemor marked this pull request as ready for review January 16, 2025 20:38
@Lendemor Lendemor merged commit 6e54652 into main Jan 16, 2025
41 checks passed
@Lendemor Lendemor deleted the lendemor/default_cache_rx_var branch January 16, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants