Support FunctionVar handlers in EventChain rendering#6188
Support FunctionVar handlers in EventChain rendering#6188FarhanAliRaza wants to merge 16 commits intoreflex-dev:mainfrom
Conversation
Allow EventChain to accept frontend FunctionVar handlers alongside EventSpec, EventVar, and EventCallback values. When a chain contains FunctionVars, keep backend events grouped through addEvents(...) and invoke frontend functions inline with the trigger arguments so mixed chains preserve execution order and DOM event actions like preventDefault and stopPropagation. Wrap inline arrow functions before emitting VarOperationCall JS so direct invocation renders valid JavaScript, add unit coverage for pure/mixed event-chain formatting and creation, and move upload exception docs to the helper that actually raises them to satisfy darglint.
Greptile SummaryThis PR adds support for Key changes:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["EventChain.create(value)"] --> B{isinstance value Var?}
B -- Yes --> C{EventChainVar?}
C -- Yes --> D["warn if kwargs\nreturn value as-is"]
C -- No --> E{FunctionVar?}
E -- Yes --> F["warn if kwargs\nreturn value as-is"]
E -- No --> G{EventVar?}
G -- Yes --> H["wrap in list → list path"]
G -- No --> I["guess type / raise"]
B -- No --> J{list?}
J -- Yes --> K["for each item in list"]
K --> L{EventHandler / EventSpec?}
L -- Yes --> M["call_event_handler → EventSpec"]
L -- No --> N{EventVar or FunctionVar?}
N -- Yes --> O["append directly to events"]
N -- No --> P{Callable / lambda?}
P -- Yes --> Q["call_event_fn → result"]
Q --> R{result is EventVar or FunctionVar?}
R -- Yes --> O
R -- No --> S["raise ValueError"]
M --> T["events list built"]
O --> T
T --> U["return EventChain(events=…)"]
U --> V["LiteralEventChainVar.create(EventChain)"]
V --> W{any FunctionVar in events?}
W -- No --> X["invocation.call(all events, arg_def, event_actions)"]
W -- Yes --> Y["build JS block statement"]
Y --> Z["emit preventDefault/stopPropagation if needed"]
Z --> AA["for each event"]
AA --> AB{FunctionVar?}
AB -- Yes --> AC["flush backend group → addEvents(…);\nevent.call(call_args);"]
AB -- No --> AD["append to queueable_group"]
AC --> AA
AD --> AA
AA --> AE["flush remaining backend group"]
AE --> AF["return_expr = Var(JS block)"]
X --> AG["return LiteralEventChainVar"]
AF --> AG
Last reviewed commit: "fix: forward non-DOM..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… function detection Event actions like `throttle` were dropped when rendering mixed EventChains containing both backend handlers and FunctionVars. Non-DOM actions are now correctly forwarded to the queueEvents call. Also fixes false-positive arrow function detection for expressions like `factory(() => 1)` that contain `=>` but are not themselves arrow functions, and adds warnings when event_chain_kwargs are silently ignored for EventChainVar/FunctionVar values.
|
Does it require documentation? |
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
The overarching theme of this review is to focus on unifying various code paths to avoid complexity and special cases that might not be covered well by tests. If there is only one path to accomplish X, then we're less likely to regress an edge case with future changes.
…ndering Move event action handling (preventDefault, stopPropagation, throttle, debounce, temporal) into a shared applyEventActions JS function used by both addEvents and Python-generated event chains. This eliminates the complex queueable-group batching logic in LiteralEventChainVar and the arrow function detection heuristic in FunctionVar.
Restore selective arrow-function wrapping in FunctionVar calls so operator-like expressions such as `typeof` keep compiling correctly. This fixes the generated JSX regression that broke form integration tests by emitting invalid code like `((typeof)(value))`. Also update unit expectations for the corrected rendering behavior.
|
The failing test is flaky |
tests/units/test_app.py
Outdated
| ) | ||
| ].strip() | ||
|
|
||
| def _wrap_event_handler_calls(handler_js: str) -> str: |
There was a problem hiding this comment.
seems like it might be better to just make these replacements in the asserted string
There was a problem hiding this comment.
seems this function can be removed?
made the fix in #6196 you can merge from |
The fast path grouped backend-only EventSpecs into a single addEvents call, which lost per-spec event actions like individual debounce values. Each EventSpec now renders its own addEvents call, and chain-level actions use applyEventActions consistently.
tests/units/test_event.py
Outdated
| assert '["debounce"] : 1000' in rendered | ||
| assert '["debounce"] : 200' in rendered |
There was a problem hiding this comment.
probably also assert that these each appear once in the rendered output
tests/units/test_event.py
Outdated
| assert '["debounce"] : 1000' in rendered | ||
| assert '["debounce"] : 200' in rendered | ||
| assert '["preventDefault"] : true' in rendered |
There was a problem hiding this comment.
similarly here, lets assert that each of these action strings appear exactly once in the code.
Without this, using `==` in an assertion with Var values ends up creating a new truthy Var instead of actually checking python-side equality. Var equality assertions have to be made explicitly with `.equals` to actually determine if the values are the same.
When a FunctionVar is passed to an event trigger with a given args_spec,
transform it into a partial function that applies the transformed arguments
when called. This allows FunctionVar handlers to work with on_blur and
on_submit, which, by default, use the args_spec to transform the value before
passing it off to the handler.
Some escape hatches:
* The behavior for EventChainVar is unchanged, so previous code that was
explicitly casting functions to EventChain will continue to work without
modification.
* If the FunctionVar is returned through a lambda, no partial application is
applied, because that happens at the point the lambda is called, so the
return value of the lambda is responsible for mapping the arguments if
desired.
This change also allows event handler lambda functions to return a
heterogeneous mix of EventSpec/EventHandler/FunctionVar (and EventChain
returned from lambda are treated as FunctionVar, allowing arbitrary nesting).
Update FunctionVar.partial such that passing no args does NOT create a new
useless function.
|
@FarhanAliRaza I added some commits to catch some additional cases that were missing while i was testing manually. Specifically code like this: rx.form(
rx.input(
name="input_value",
on_blur=lambda v: rx.EventChain.create(
value=log_after_timeout.partial(v.to(str).capitalize()),
args_spec=lambda: (),
),
),
rx.button(
"Submit",
),
on_submit=[
log_after_timeout,
lambda fd: [
log_after_timeout.partial(fd.to(dict)["input_value"]),
rx.console_log(fd.to(dict)["input_value"]),
],
State.on_submit,
],
),2 behavior changes:
Additionally, lambda return value validation was consolidated into Can you explore more edge cases around this and add some additional test cases to the unit tests suite. |
this allows rx.cond to continue to be used for conditional events
|
test failure was from another interesting case that we don't have a unit test for yet on_key_down=lambda key: rx.cond(
rx.Var.create(["a", "s", "d", "w"]).contains(key),
GlobalHotkeyState.update_key(key),
rx.console_log(key)
)here the event handler is a lambda that returns some conditional code that runs on the frontend before deciding whether to trigger a backend event or do something else. Stuff like this still doesn't work though rx.input(
name="input_value",
on_blur=lambda v: rx.cond(
v == "foo",
log_after_timeout.partial("Input was foo!"),
State.do_a_thing(v),
),
),When we have an |
|
I fixed it locally by adding this. Is it correct? elif isinstance(e, Var) and safe_issubclass(
e._var_type, (EventSpec, EventChain)
):
# Helpers like rx.cond() can return event-typed Vars that should be
# treated the same as literal EventSpec/EventChain values here.
e = e.guess_type()
Update: |
I think we have some special logic somewhere that is treating the I think we can just defer on this problem for now; it wasn't working before and still doesn't work now. We could improve it in the future. |
reflex/event.py
Outdated
| if not statements: | ||
| statements.append(invocation.call(LiteralVar.create([]), arg_def_expr, {})) | ||
|
|
||
| statement_var_data = [statement._get_all_var_data() for statement in statements] |
There was a problem hiding this comment.
i think this might be the line that is causing the CodSpeed regression.
usually if you just combine the vars, the parser will pick up any nested VarData automatically:
statement_block = Var(
_js_expr=f"{{{''.join(f'{statement};' for statement in statements)}}}",
)i would expect the above to be equivalent to how it was written without the explicit VarData merge
There was a problem hiding this comment.
You were right, it is fixed now.
…in statement block Cache empty event/action literals as module-level constants to avoid repeated allocations. Remove the manual _get_all_var_data merge on the statement block Var so nested var data propagates naturally through the f-string interpolation.
2 participants
Allow EventChain to accept frontend FunctionVar handlers alongside EventSpec, EventVar, and EventCallback values.
When a chain contains FunctionVars, keep backend events grouped through addEvents(...) and invoke frontend functions inline with the trigger arguments so mixed chains preserve execution order and DOM event actions like preventDefault and stopPropagation.
Wrap inline arrow functions before emitting VarOperationCall JS so direct invocation renders valid JavaScript, add unit coverage for pure/mixed event-chain formatting and creation, and move upload exception docs to the helper that actually raises them to satisfy darglint.
All Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features:
closes #6185