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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

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.

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

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