Support FunctionVar handlers in EventChain rendering#6188
Support FunctionVar handlers in EventChain rendering#6188FarhanAliRaza wants to merge 6 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}
| func_expr = str(self._func) | ||
| if "=>" in func_expr and not format.is_wrapped(func_expr, "("): | ||
| func_expr = format.wrap(func_expr, "(") |
There was a problem hiding this comment.
=> detection is unreliable for complex expressions
The check "=>" in func_expr is a coarse string scan that fires whenever => appears anywhere in the serialised expression — including inside nested arrow functions in function bodies (e.g. (...args) => { const f = x => x + 1; return f(args); }) or inside template literals / string arguments.
Although wrapping an already-valid expression in extra parentheses is harmless for pure arrow-function strings (JavaScript treats (expr) the same as expr), a false positive on a non-arrow-function expression that happens to contain the => character sequence could generate unexpected JS output in edge cases.
A more robust approach would be to track whether a FunctionVar is an inline arrow function at construction time (e.g. a boolean flag _is_arrow_fn on the FunctionVar/FunctionStringVar), or at minimum restrict the heuristic to expressions that begin with an arrow-function parameter list:
import re
_ARROW_FN_RE = re.compile(r"^\s*(\(.*?\)|[A-Za-z_$][A-Za-z0-9_$]*)\s*=>")
func_expr = str(self._func)
if _ARROW_FN_RE.match(func_expr) and not format.is_wrapped(func_expr, "("):
func_expr = format.wrap(func_expr, "(")
reflex/event.py
Outdated
| def flush_queueable_group() -> None: | ||
| if not queueable_group: | ||
| return | ||
| queue_call = invocation.call( | ||
| LiteralVar.create([ | ||
| LiteralVar.create(event) for event in queueable_group | ||
| ]), | ||
| arg_def_expr, | ||
| {}, | ||
| ) | ||
| statement_js.append(f"{queue_call!s};") | ||
| statement_var_data.append(queue_call._get_all_var_data()) | ||
| queueable_group.clear() |
There was a problem hiding this comment.
invocation is unsafely used without a null guard inside closure
flush_queueable_group is a closure that calls invocation.call(...) on line 2144. At the point where the closure is defined, the type of invocation (from the perspective of both the type checker and a careful reader) is FunctionVar | None — the raise ValueError guard above only eliminates the case invocation is not None and not FunctionVar, leaving None as a valid residual type.
In practice invocation is always non-None here (it was just set a few lines earlier), but the closure captures the variable binding, not a guaranteed-non-None value. A type checker will flag invocation.call(...) as a potential None dereference, and the code is brittle if the assignment logic above ever changes.
Add an assert invocation is not None before the closure definition, or restructure to pass invocation as a parameter:
assert invocation is not None # guaranteed non-None by the assignments above
def flush_queueable_group() -> None:
if not queueable_group:
return
queue_call = invocation.call( # type checker now knows it's FunctionVar
...
)
reflex/event.py
Outdated
| else: | ||
| statement_js: list[str] = [] | ||
| statement_var_data: list[VarData | None] = [] | ||
| queueable_group: list[EventSpec | EventVar | EventCallback] = [] | ||
|
|
||
| if value.event_actions.get("preventDefault") or value.event_actions.get( | ||
| "stopPropagation" | ||
| ): | ||
| statement_js.append( | ||
| "const _reflex_dom_event = " | ||
| f"{arg_def_expr}.filter((o) => o?.preventDefault !== undefined)[0];" | ||
| ) | ||
| if value.event_actions.get("preventDefault"): | ||
| statement_js.append( | ||
| "if (_reflex_dom_event?.preventDefault) " | ||
| "{_reflex_dom_event.preventDefault();}" | ||
| ) | ||
| if value.event_actions.get("stopPropagation"): | ||
| statement_js.append( | ||
| "if (_reflex_dom_event?.stopPropagation) " | ||
| "{_reflex_dom_event.stopPropagation();}" | ||
| ) | ||
|
|
||
| def flush_queueable_group() -> None: | ||
| if not queueable_group: | ||
| return | ||
| queue_call = invocation.call( | ||
| LiteralVar.create([ | ||
| LiteralVar.create(event) for event in queueable_group | ||
| ]), | ||
| arg_def_expr, | ||
| {}, | ||
| ) | ||
| statement_js.append(f"{queue_call!s};") | ||
| statement_var_data.append(queue_call._get_all_var_data()) | ||
| queueable_group.clear() | ||
|
|
||
| for event in value.events: | ||
| if isinstance(event, FunctionVar): | ||
| flush_queueable_group() | ||
| function_call = event.call(*call_args) | ||
| statement_js.append(f"{function_call!s};") | ||
| statement_var_data.append(function_call._get_all_var_data()) | ||
| else: | ||
| queueable_group.append(event) | ||
|
|
||
| flush_queueable_group() | ||
|
|
||
| return_expr = Var( | ||
| _js_expr=f"{{{''.join(statement_js)}}}", | ||
| _var_data=VarData.merge(*statement_var_data), |
There was a problem hiding this comment.
preventDefault/stopPropagation event actions silently dropped in mixed mode
In the mixed-case branch (has_function_var = True), flush_queueable_group always passes {} as event_actions to addEvents. Only preventDefault and stopPropagation are explicitly re-implemented via manual DOM calls at the top of the block.
If value.event_actions ever contains keys beyond those two (e.g. a future action type, or user-supplied data), they are silently discarded in every addEvents(...) call within the mixed path. This is in contrast to the pure-EventSpec path (line 2116) which correctly passes value.event_actions through.
Consider forwarding any remaining event_actions to addEvents:
# Strip the DOM-managed keys; forward everything else to addEvents
remaining_actions = {
k: v
for k, v in value.event_actions.items()
if k not in ("preventDefault", "stopPropagation")
}
# then in flush_queueable_group:
queue_call = invocation.call(
LiteralVar.create([...]),
arg_def_expr,
remaining_actions, # was hardcoded {}
)| if isinstance(value, Var): | ||
| if isinstance(value, EventChainVar): | ||
| return value | ||
| if isinstance(value, FunctionVar): | ||
| return value |
There was a problem hiding this comment.
event_chain_kwargs silently ignored for bare FunctionVar inputs
When value is a FunctionVar, create returns it directly without applying event_chain_kwargs (which may include invocation, custom event_actions, etc.). This means callers that do:
EventChain.create(some_fn_var, args_spec=..., event_actions={"preventDefault": True})will silently get back some_fn_var with none of those kwargs applied. The discrepancy is not documented and could confuse users who expect event_actions to be respected for FunctionVar inputs.
At minimum this behaviour should be documented with a comment explaining the intentional early-return, or a warning should be emitted if event_chain_kwargs is non-empty:
if isinstance(value, FunctionVar):
# FunctionVar is its own event handler; event_chain_kwargs are intentionally ignored.
if event_chain_kwargs:
warnings.warn(
f"event_chain_kwargs {event_chain_kwargs!r} are ignored for FunctionVar values.",
stacklevel=2,
)
return value… 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.
| """ | ||
| return f"({self._func!s}({', '.join([str(LiteralVar.create(arg)) for arg in self._args])}))" | ||
| func_expr = str(self._func) | ||
| if _starts_with_arrow_function(func_expr) and not format.is_wrapped( |
There was a problem hiding this comment.
do we really need the complex logic to detect an arrow function or can we just unconditionally wrap the expression in parentheses if it's not already wrapped
There was a problem hiding this comment.
It breaks the integration test without this.
There was a problem hiding this comment.
It emitts invalid code like ((typeof)(value))
reflex/event.py
Outdated
| if value.event_actions.get("preventDefault") or value.event_actions.get( | ||
| "stopPropagation" | ||
| ): | ||
| statement_js.append( | ||
| "const _reflex_dom_event = " | ||
| f"{arg_def_expr}.filter((o) => o?.preventDefault !== undefined)[0];" | ||
| ) | ||
| if value.event_actions.get("preventDefault"): | ||
| statement_js.append( | ||
| "if (_reflex_dom_event?.preventDefault) " | ||
| "{_reflex_dom_event.preventDefault();}" | ||
| ) | ||
| if value.event_actions.get("stopPropagation"): | ||
| statement_js.append( | ||
| "if (_reflex_dom_event?.stopPropagation) " | ||
| "{_reflex_dom_event.stopPropagation();}" | ||
| ) |
There was a problem hiding this comment.
code for handling the event actions (with the addition of throttle and debounce) already exists in state.js, but it's inside the addEvents functions. i would suggest pulling this logic out as an exported function in state.js that takes the event arg and a function which could either be an addEvents partial or some user-passed function.
instead of collecting all the event actions and combining them into the EventChain, it probably is better structure to leave the event actions on the individual EventSpec, and apply any further event actions to the event chain itself. Then use the same function to process these actions at each level.
For example, you might have applyEventActions(target, event_actions, ...args); in this case the target could be the entire EventChain function if the chain has its own actions, or the actions could be passed to each addEvents call (which internally would call applyEventActions).
reflex/event.py
Outdated
| stacklevel=2, | ||
| ) | ||
| return value | ||
| if isinstance(value, EventVar): |
There was a problem hiding this comment.
include FunctionVar here and remove the special case for it above. We should now be treating EventVar and FunctionVar in the same way: creating a new event chain that includes these values in the events list so they can be composed and formatted as an EventChainVar later.
reflex/event.py
Outdated
|
|
||
| has_function_var = any(isinstance(e, FunctionVar) for e in value.events) | ||
|
|
||
| if not has_function_var: |
There was a problem hiding this comment.
we should strive to reduce special cases in this function. i think it's absolutely fine if we process both mixed and homogenous cases with the same logic.
it's actually better if we have a separate addEvents call for each EventSpec because then each of those can have different event actions attached to them and the system is more composable.
reflex/event.py
Outdated
| statement_js.append(f"{function_call!s};") | ||
| statement_var_data.append(function_call._get_all_var_data()) | ||
| else: | ||
| queueable_group.append(event) |
There was a problem hiding this comment.
just to reiterate, i don't think we need or want to batch up these groups; each non-FunctionVar should get its own addEvents invocation.
…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 |
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