Skip to content

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?

@maartenbreddels maartenbreddels added this to the Solara 2.0 milestone Apr 11, 2024
@render
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).

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
Copy link
Contributor Author

Above TODO's are not an issue, since this is an opt-in feature now.

@maartenbreddels maartenbreddels marked this pull request as ready for review December 3, 2024 13:59
Copy link
Collaborator

@iisakkirotko iisakkirotko left a comment

Choose a reason for hiding this comment

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

Looks quite good! Looking forward to merging this soon

@maartenbreddels maartenbreddels temporarily deployed to feat_mutation_detection_reactive_vars - solara-stable PR #595 December 4, 2024 13:34 — with Render Destroyed
@iisakkirotko iisakkirotko force-pushed the feat_mutation_detection_reactive_vars branch from 7279637 to 9b46f75 Compare December 4, 2024 13:37
@iisakkirotko iisakkirotko force-pushed the feat_mutation_detection_reactive_vars branch from 9b46f75 to 84b293a Compare December 4, 2024 14:24
@maartenbreddels maartenbreddels temporarily deployed to feat_mutation_detection_reactive_vars - solara-stable PR #595 December 4, 2024 14:24 — with Render Destroyed
By using `sys._getframe` when possible, we should make quite substantial performance gains over using inspect
@iisakkirotko iisakkirotko force-pushed the feat_mutation_detection_reactive_vars branch from 84b293a to 84faf0c Compare December 4, 2024 14:28
@maartenbreddels maartenbreddels temporarily deployed to feat_mutation_detection_reactive_vars - solara-stable PR #595 December 4, 2024 14:28 — with Render Destroyed
@iisakkirotko iisakkirotko merged commit abcfdf8 into master Dec 4, 2024
27 checks passed
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.

4 participants