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

feat: detect mutation to values of reactive vars #595

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maartenbreddels
Copy link
Contributor

@maartenbreddels maartenbreddels commented Apr 11, 2024

A common source of error is mutation of values in reactive vars. The reactive var cannot notice a change in its value (e.g. a list) if the list is mutated in place.

A user can be mislead that it is working correctly, when another reactive variable triggers a rerender, teaching bad habits.

Detecting mutations in Python without using proxies can be done by making a deepcopy of the object, and comparing the reference to the deepcopy.

This comes at the cost of performance and memory usage, therefore we should enabled this by default in development mode (tests run in this mode by default), so app builders are aware of mistakes while developing.
In production mode we can disable the mutation checks and behave as before.

TODO:

  • Trigger calls to check_mutations() from several places: after an event handler (requires a change in reacton) and after a component run.
  • We probably do not want two equals function in solara/reacton, reconcile this.
  • Give an option to opt out of the mutation check per reactive var (e.g. when equals fails), or for performance reasons.
  • support reactive.get(copy=True) to always get a copy, even when _CHECK_MUTATIONS is False
  • Do we need support reactive.get(reference=True) to always get a reference, or is the opt-out enough?

A common source of error is mutation of values in reactive vars.
The reactive var cannot notice a change in its value (e.g. a list)
if the list is mutated in place.

A user can be mislead that it is working correctly, when another
reactive variable triggers a rerender, teaching bad habits.

Detecting mutations in Python without using proxies can be done by
making a deepcopy of the object, and comparing the reference to the
deepcopy.

This comes at the cost of performance and memory usage, therefore we
should enabled this by default in development mode, so app builders
are aware of mistakes while developing.
In production mode we can disable the mutation checks and behave as
before.
@maartenbreddels maartenbreddels added this to the Solara 2.0 milestone Apr 11, 2024
Copy link

render bot commented Apr 11, 2024

@Corvince
Copy link

I don't know if this is the right place to discuss this, but since this PR bundles several issues, I'll give it a try hear.

First of all I would question using mutable data is per-se a bad habit. I think there a valid use-cases and in generally it can be much more intuitive. Maybe you can write a bit about why mutable data is bad in the context of solara.

Second, I worked the past year with Vue.js a lot and from that direction I now find the name very confusing. In vuejs reactive(foo) somewhat does the opposite from solara. It wraps any object into a mutable proxy, but it does not work on primitive values. In solara it works with any variable, but its much more intuitive with simple types, since they are immutable and so you can't accidentally mutate the state (which this PR addresses). So where does the name "reactive" come from and why not for example signals?

Lastly, I like the approach of Solid.js, where you can create a signal with the option {equal: false}, which just always updates when the set function is called. This would allow mutable data with a clear trigger for updates.

Hope my question and comments are helpful

@maartenbreddels
Copy link
Contributor Author

Lastly, I like the approach of Solid.js, where you can create a signal with the option {equal: false}

Very interesting idea, and would solve #245

I'll start writing about this topic (hopefully soon).

@maartenbreddels
Copy link
Contributor Author

Second, I worked the past year with Vue.js a lot and from that direction I now find the name very confusing.

Vue's refs and Solara reactive's are signals. I personally don't like the name signals at all (very technical name), but I think it would be less confusing in the end if Vue and Solara used signals as the name.

Vue's reactive (a proxy around a signal/ref), can be done in Python as well, and I'm mentally almost there... :)

I would say mutation is not necessarily bad, but it's difficult to reason about code if lists, etc., are mutated while parallel written into a database (in a different thread). The code might crash if another thread is iterating over a list while another thread is mutating it. You may not have stored in the database what you thought, etc.

To understand a larger codebase, data mutation makes it more difficult to reason about your code because you need to know who mutates where and when.

So basically, we say, don't mutate your state/data, mutate the signals/reactives, since those can also be 'observed' (i.e. we know we need to rerender, and we do not have the risk of our UI being out of sync with the state).

We plan to support a custom equals operator (which could always return False to support #245), but there is a small caviat there as well: If the same (mutated) objects/arguments are also passed to a child component, we assume that the component does not need a rerender. So, to support mutating objects we also need to support custom equals for components (which we plan to do).

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.

2 participants