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

[REF-2273] Implement .setvar special EventHandler #3163

Merged
merged 6 commits into from
May 2, 2024
Merged

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Apr 25, 2024

All State subclasses will now have a special setvar EventHandler which appears in the autocomplete drop down, passes static analysis, and can be used to set State Vars in response to event triggers.

Before:

rx.input(value=State.a, on_change=State.set_a)

After:

rx.input(value=State.a, on_change=State.setvar("a"))

This reduces the "magic" because setvar is statically defined on all State subclasses.


To make this actually work nicely, this PR also enables partial application of EventHandler.

When an EventHandler is called with an incomplete set of args it creates a partial EventSpec. This change allows Component._create_event_chain to apply remaining args from an args_spec to an existing EventSpec to make it functional.

Instead of requiring the use of lambda functions to pass arguments to an EventHandler, they can now be passed directly and any remaining args defined in the event trigger will be applied after those.


Deprecations: this removes the old-style args_spec which hasn't been used since 0.2.x and was slated for removal in 0.5.0.

When an EventHandler is called with an incomplete set of args it creates a
partial EventSpec. This change allows Component._create_event_chain to apply
remaining args from an args_spec to an existing EventSpec to make it
functional.

Instead of requiring the use of `lambda` functions to pass arguments to an
EventHandler, they can now be passed directly and any remaining args defined in
the event trigger will be applied after those.
All State subclasses will now have a special `setvar` EventHandler which
appears in the autocomplete drop down, passes static analysis, and canbe used
to set State Vars in response to event triggers.

Before:
    rx.input(value=State.a, on_change=State.set_a)

After:
    rx.input(value=State.a, on_change=State.setvar("a"))

This reduces the "magic" because `setvar` is statically defined on all State
subclasses.
Copy link

linear bot commented Apr 25, 2024

@masenf
Copy link
Collaborator Author

masenf commented Apr 26, 2024

When using setvar with a non-existing state var, this is actually a runtime error instead of a compile time error. That may be non-ideal behavior.

Maybe instead of setvar, we should expose a .set EventSpec on each BaseVar instead.

@benedikt-bartscher
Copy link
Contributor

@masenf how about this?

rx.input(value=State.a, on_change=State.setvar(State.a))

@masenf
Copy link
Collaborator Author

masenf commented Apr 29, 2024

@benedikt-bartscher we considered that, but it's actually kind of ambiguous... is it setting State.a itself, or is it an indirection that sets the var name referenced by State.a.

class State(rx.State):
    a: str = "b"
    b: str = "foo"

...

rx.input(value=State.a, on_change=State.setvar(State.a))

Does this set State.a to the value of the input... or does it set State.b to the value of the input (because State.a is "b").

@benedikt-bartscher
Copy link
Contributor

benedikt-bartscher commented Apr 29, 2024

You may be right, it is kind of ambiguous... I wanted to find a typed alternative to setvar("a") and this was the first notation that came into my mind.

Maybe instead of setvar, we should expose a .set EventSpec on each BaseVar instead.

I like this one. It is not that ambiguous, and "typed".

@masenf
Copy link
Collaborator Author

masenf commented Apr 29, 2024

So i tried the `State.a.set` thing
diff --git a/reflex/state.py b/reflex/state.py
index a84ea7f8..2845d280 100644
--- a/reflex/state.py
+++ b/reflex/state.py
@@ -909,6 +909,7 @@ class BaseState(Base, ABC, extra=pydantic.Extra.allow):
         if setter_name not in cls.__dict__:
             event_handler = cls._create_event_handler(prop.get_setter())
             cls.event_handlers[setter_name] = event_handler
+            prop._set = event_handler
             setattr(cls, setter_name, event_handler)
 
     @classmethod
diff --git a/reflex/vars.py b/reflex/vars.py
index 4a8e6b30..8268d003 100644
--- a/reflex/vars.py
+++ b/reflex/vars.py
@@ -40,6 +40,7 @@ from reflex.utils import console, format, imports, serializers, types
 from reflex.utils.imports import ImportDict, ImportVar
 
 if TYPE_CHECKING:
+    from reflex.event import EventHandler
     from reflex.state import BaseState
 
 # Set of unique variable names.
@@ -1739,6 +1740,9 @@ class BaseVar(Var):
     # Extra metadata associated with the Var
     _var_data: Optional[VarData] = dataclasses.field(default=None)
 
+    # An EventHandler for setting the Var from an event trigger
+    _set: "EventHandler" | None = dataclasses.field(default=None)
+
     def __hash__(self) -> int:
         """Define a hash function for a var.
 
@@ -1835,6 +1839,19 @@ class BaseVar(Var):
 
         return setter
 
+    @property
+    def set(self) -> "EventHandler":
+        """Get the EventHandler for setting the var.
+
+        Returns:
+            An EventHandler for setting the var.
+        """
+        if self._set is None:
+            raise ValueError(
+                f"Var {self._var_full_name} does not have a setter defined."
+            )
+        return self._set
+
 
 @dataclasses.dataclass(init=False, eq=False)
 class ComputedVar(Var, property):
diff --git a/reflex/vars.pyi b/reflex/vars.pyi
index fb2ed465..51d68b98 100644
--- a/reflex/vars.pyi
+++ b/reflex/vars.pyi
@@ -6,6 +6,7 @@ from dataclasses import dataclass
 from _typeshed import Incomplete
 from reflex import constants as constants
 from reflex.base import Base as Base
+from reflex.event import EventHandler as EventHandler
 from reflex.state import State as State
 from reflex.state import BaseState as BaseState
 from reflex.utils import console as console, format as format, types as types
@@ -126,10 +127,13 @@ class BaseVar(Var):
     _var_is_string: bool = False
     _var_full_name_needs_state_prefix: bool = False
     _var_data: VarData | None = None
+    _set: EventHandler | None = None
     def __hash__(self) -> int: ...
     def get_default_value(self) -> Any: ...
     def get_setter_name(self, include_state: bool = ...) -> str: ...
     def get_setter(self) -> Callable[[BaseState, Any], None]: ...
+    @property
+    def set(self) -> EventHandler: ...
 
 @dataclass(init=False)
 class ComputedVar(Var):
diff --git a/tests/test_state.py b/tests/test_state.py
index ce62e9c6..c13f3a64 100644
--- a/tests/test_state.py
+++ b/tests/test_state.py
@@ -2883,3 +2883,29 @@ async def test_setvar(mock_app: rx.App, token: str):
     # Cannot setvar with non-string
     with pytest.raises(ValueError):
         TestState.setvar(42, 42)
+
+
+@pytest.mark.asyncio
+async def test_base_var_set(mock_app: rx.App, token: str):
+    """Test that Var.set works correctly.
+
+    Args:
+        mock_app: An app that will be returned by `get_app()`
+        token: A token.
+    """
+    state = await mock_app.state_manager.get_state(_substate_key(token, TestState))
+
+    # Set Var in same state (with Var type casting)
+    for event in rx.event.fix_events(
+        [TestState.num1.set(42), TestState.num2.set("4.2")], token
+    ):
+        async for update in state._process(event):
+            print(update)
+    assert state.num1 == 42
+    assert state.num2 == 4.2
+
+    # Set Var in parent state
+    for event in rx.event.fix_events([GrandchildState.array.set([43])], token):
+        async for update in state._process(event):
+            print(update)
+    assert state.array == [43]
And while it does work, it still does not pass typing because the Vars in a state are treated like the actual type they are (`str`, `bool`, `int`, etc). Statically, they are not known as `Var`, so the `set` property still throws a typing error.
/Users/masenf/code/reflex-dev/reflex/tests/test_state.py
  /Users/masenf/code/reflex-dev/reflex/tests/test_state.py:2900:25 - error: Cannot access member "set" for type "int"
    Member "set" is unknown (reportGeneralTypeIssues)
  /Users/masenf/code/reflex-dev/reflex/tests/test_state.py:2900:49 - error: Cannot access member "set" for type "float"
    Member "set" is unknown (reportGeneralTypeIssues)
  /Users/masenf/code/reflex-dev/reflex/tests/test_state.py:2908:61 - error: Cannot access member "set" for type "List[float]"
    Member "set" is unknown (reportGeneralTypeIssues)

Ultimately, we need to make Var and EventHandler into descriptors so that we can statically type the State class (and get rid of a bunch of dodgy __init_subclass__, __getattribute__ and __setattr__ logic along the way as well). But that's a much bigger project.

Probably will take this change as-is for now, and circle back around to the typing improvements once we have real descriptors for these fields.

This week, the focus is on getting a 0.5.0 release out with radix 3.0 and some other Component API changes, so any descriptor work would have to wait a few weeks at minimum.

@benedikt-bartscher
Copy link
Contributor

benedikt-bartscher commented Apr 29, 2024

Yes, I know that it won't pass pyright for base_vars, but (as you explained) this is another issue not related to your State.a.set implementation. It should pass pyright with ComputedVars though. I would prefer implementing a typed variant directly and annotate the related lines with ignore comments (like we already need to do for other problems introduced by the __init_subclass__ magic).
My main intention is to reduce reflex's api changes.

I am really looking forward to descriptor based event handlers and vars 🚀


# Set Var in same state (with Var type casting)
for event in rx.event.fix_events(
[TestState.setvar("num1", 42), TestState.setvar("num2", "4.2")], token
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the way to set the num2 to a string value of "4.2" here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

num2 is typed as a float, you can't (shouldn't) set it to a str

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, should've looked at the hidden part of the code. That's what the type casting is. Sounds good.

@masenf masenf merged commit c636c91 into main May 2, 2024
46 checks passed
@masenf masenf deleted the masenf/state-setvar branch June 25, 2024 02:21
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.

None yet

3 participants