Skip to content

Support raw JavaScript events #1289

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.

Sign up for GitHub

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

Closed
wants to merge 14 commits into from
Closed

Support raw JavaScript events #1289

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
13f588a
Update vdom.py
shawncrawley Mar 28, 2025
edf0498
Update layout.py
shawncrawley Mar 28, 2025
8ae45ce
Update vdom.tsx
shawncrawley Mar 28, 2025
c11c2c4
Update react.js
shawncrawley Mar 28, 2025
db770ac
Add missing parenthesis
shawncrawley Mar 28, 2025
67cd1e7
Update layout.py
shawncrawley Mar 28, 2025
36e33e4
Update vdom.py
shawncrawley Mar 28, 2025
61fc1d3
Adds test
shawncrawley Mar 29, 2025
258c4de
Update types.py
shawncrawley Mar 29, 2025
5ce5c32
Update types
shawncrawley Mar 29, 2025
0b14bc8
Add one more test
shawncrawley Mar 29, 2025
23a7297
Implement JavaScript type and handle eval
shawncrawley Mar 31, 2025
a109a3b
Fix broken test
shawncrawley Mar 31, 2025
3a159af
Replaces test for callable non-event prop
shawncrawley Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/js/packages/@reactpy/client/src/vdom.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,9 @@ function createEventHandler(
name: string,
{ target, preventDefault, stopPropagation }: ReactPyVdomEventHandler,
): [string, () => void] {
if (target.indexOf("javascript:") == 0) {
return [name, eval(target.replace("javascript:", ""))];
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this statement would result in the eval being run instantly, rather than when the event occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I didn't realize it, apparently my first stab at this was with the only design that would actually work for this: using arrow function expressions.

let test = eval(`() => console.log("Hello World")`);
test()

The above assigns the arrow function to the test variable, which can then be executed.

In what ways does this fall short? I need to think through that myself... I did just check that simply using a function name works:

function myFunction () {
    console.log("Hello World");
}
let test = eval("myFunction");
test()

Any concerns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still cannot come up with a case where you would need anything other than a direct function name or an arrow function expression. All on<CamelCaseText> event handlers are passed an event argument at a minimum, which is often relevant to the processing. For example:

html.div({"onClick": '(e) => e.target.innerText = "Thank you!"'}, "Click Me!")

If we wanted to execute eval(<javascript_text>) at the very time the event is being fired, the above example would no longer work. If that were the case, how would I implement the above? Like this?

html.div({"onClick": 'e.target.innerText = "Thank you!"'}, "Click Me!")

Unfortunately, that would not work unless the user knows exactly what the "event" variable will be called in the scope in which it's called - which would have to be e for the example above to work. Sure, we could document that. But it seems more rigid and less appropriate.

Thus, again, I'm pushing for either a function name or an arrow function expression.

Copy link
Contributor

@Archmonger Archmonger Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Rather than expecting e.target.innerText scenarios for plain text events...

html.div({"onClick": 'e.target.innerText = "Thank you!"'}, "Click Me!")

We really should be expecting something more akin to plain HTML inline JavaScript, where the appropriate context is provided via this (for example, `this.innerText)...

html.div({"onClick": 'this.innerText = "Thank you!"'}, "Click Me!")

In JavaScript, you can manipulate the value of this for an eval statement by creating a simple wrapper.

function evalWithThis(code, thisValue) {
    return function() {
        return eval(code);
    }.call(thisValue);
}

// Example
const code = "this.message";
const context = { message: "Hello, world!" };
console.log(evalWithThis(code, context)); // Outputs: "Hello, world!"

The end goal is that this piece of test code, which is valid HTML, should be able to work when used within ReactPy.

# Note that `string_to_reactpy` was previously named `html_to_vdom` in ReactPy v1
from reactpy import string_to_reactpy

@component
def example():
    return string_to_reactpy("""<button onclick='this.innerText = "COMPLETE"'> Click Me </button>""")

We can attach our own special properties onto this to give some more context to the event if needed. I could imagine that we would create a this.event property that has all our typical event data into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That all makes sense when looking at ReactPy as strictly a way to create valid HTML. However, I'm still stuck on my main use case - which is using ReactPy as a wrapper around an existing ReactJS component library: AG Grid. The way I've initially designed this fixes a bug I've had for months with a web page I'm building.

I have a grid with "Single Row Selection" enabled, which renders a checkbox with each row. I want to render a chart whenever a row is selected via clicking (i.e. "checking") the checkbox, so I also configure the "onRowSelected" event that pulls the selected row from the event and updates the ReactPy state with set_show_chart(row_id). This state update causes the table to re-render the rows, which removes the check inside of the checkbox on the row that was selected.

I dug forever wondering why the chart was being re-rendered. I found that the whole table wasn't being updated in the DOM, but only the rows. Then I found this section in their documentation about "Updating Row Data" and "Row IDs".

What I've implemented thus far in this PR finally fixed my bug by allowing me to implement this (shown with props as kwargs rather than a props dict):

AgGridReact(
    style=table_style,
    rowData=row_data,
    columnDefs=column_defs,
    defaultColDef=default_col_def,
    selection=table_selection,
    onRowSelected=handle_row_selected,
    getRowId="javascript: (params) => String(params.data.id);"
)

I do recognize that getRowId is clearly not an event listener. So yeah, I'm grasping at a way to not only support standardized events, but also any Component property that expects a callable.

Above you said:

We can attach our own special properties onto this to give some more context to the event if needed. I could imagine that we would create a this.event property that has all our typical event data into it.

Are you suggesting we could capture any/all arguments that are actually passed to the function and then attach them to our wrapped context and make them accessible via this? To generally support any callable, not just event-based, could we store the call args onto this with something like this.args?

OR! I just found this via a google search: the arguments object. I didn't realize arguments is a reserved and standard context variable, much like this. So maybe your eval solution would work here with something like getRowId="String(arguments[0].data.id); I'll go test that out.

However, I will say that the main thing that bothers me about putting statements as the JavaScriptText rather than an actual function expression is that it feels less React-y... And somewhat breaks the intuitive nature for a developer (like me, at least). Could we not somehow support both? This comes down to where/how the eval is actually called. Could we do this:

function evalWithThis(code, thisValue) {
    return function() {
        let storeFuncOrExecuteStatements = eval(code);
        if (typeof storeFuncOrExecuteStatements == "function") {
            return storeFuncOrExecuteStatements();
        }
    }.call(thisValue);
}

Sorry for the big stream-of-consciousness... It helps me to document my thought process. Thanks for listening lol. I'll go play with these ideas and await any further from you, @Archmonger.

Copy link
Contributor

@Archmonger Archmonger Mar 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As you suggested above, I'm completely ok with singleton functions (such as JavaScriptText("(params) => String(params.data.id);") ) being provided to the web module prop as a callable.

That's the most logical thing to do, especially given the fact that the eval statement would otherwise do absolutely nothing.

}
return [
name,
function (...args: any[]) {
Expand Down
32 changes: 23 additions & 9 deletions src/reactpy/core/layout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ async def deliver(self, event: LayoutEventMessage | dict[str, Any]) -> None:
# we just ignore the event.
handler = self._event_handlers.get(event["target"])

if handler is not None:
if handler is not None and not isinstance(handler, str):
try:
await handler.function(event["data"])
except Exception:
Expand Down Expand Up @@ -277,16 +277,23 @@ def _render_model_attributes(

model_event_handlers = new_state.model.current["eventHandlers"] = {}
for event, handler in handlers_by_event.items():
if event in old_state.targets_by_event:
target = old_state.targets_by_event[event]
if isinstance(handler, str):
target = handler
prevent_default = False
stop_propagation = False
else:
target = uuid4().hex if handler.target is None else handler.target
prevent_default = handler.prevent_default
stop_propagation = handler.stop_propagation
if event in old_state.targets_by_event:
target = old_state.targets_by_event[event]
else:
target = uuid4().hex if handler.target is None else handler.target
new_state.targets_by_event[event] = target
self._event_handlers[target] = handler
model_event_handlers[event] = {
"target": target,
"preventDefault": handler.prevent_default,
"stopPropagation": handler.stop_propagation,
"preventDefault": prevent_default,
"stopPropagation": stop_propagation,
}

return None
Expand All @@ -301,13 +308,20 @@ def _render_model_event_handlers_without_old_state(

model_event_handlers = new_state.model.current["eventHandlers"] = {}
for event, handler in handlers_by_event.items():
target = uuid4().hex if handler.target is None else handler.target
if isinstance(handler, str):
target = handler
prevent_default = False
stop_propagation = False
else:
target = uuid4().hex if handler.target is None else handler.target
prevent_default = handler.prevent_default
stop_propagation = handler.stop_propagation
new_state.targets_by_event[event] = target
self._event_handlers[target] = handler
model_event_handlers[event] = {
"target": target,
"preventDefault": handler.prevent_default,
"stopPropagation": handler.stop_propagation,
"preventDefault": prevent_default,
"stopPropagation": stop_propagation,
}

return None
Expand Down
6 changes: 4 additions & 2 deletions src/reactpy/core/vdom.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,13 +216,15 @@ def separate_attributes_and_event_handlers(
attributes: Mapping[str, Any],
) -> tuple[VdomAttributes, EventHandlerDict]:
_attributes: VdomAttributes = {}
_event_handlers: dict[str, EventHandlerType] = {}
_event_handlers: dict[str, EventHandlerType | str] = {}

for k, v in attributes.items():
handler: EventHandlerType
handler: EventHandlerType | str

if callable(v):
handler = EventHandler(to_event_handler_function(v))
elif isinstance(v, str) and v.startswith("javascript:"):
handler = v
elif isinstance(v, EventHandler):
handler = v
else:
Expand Down
2 changes: 1 addition & 1 deletion src/reactpy/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ class EventHandlerType(Protocol):
EventHandlerMapping = Mapping[str, EventHandlerType]
"""A generic mapping between event names to their handlers"""

EventHandlerDict: TypeAlias = dict[str, EventHandlerType]
EventHandlerDict: TypeAlias = dict[str, EventHandlerType | str]
"""A dict mapping between event names to their handlers"""


Expand Down
6 changes: 5 additions & 1 deletion src/reactpy/web/templates/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ function wrapEventHandlers(props) {
const newProps = Object.assign({}, props);
for (const [key, value] of Object.entries(props)) {
if (typeof value === "function") {
newProps[key] = makeJsonSafeEventHandler(value);
if (value.toString().includes(".sendMessage")) {
newProps[key] = makeJsonSafeEventHandler(value);
} else {
newProps[key] = value;
}
}
}
return newProps;
Expand Down
33 changes: 33 additions & 0 deletions tests/test_core/test_events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,3 +221,36 @@ def outer_click_is_not_triggered(event):
await inner.click()

await poll(lambda: clicked.current).until_is(True)


async def test_javascript_event(display: DisplayFixture):
@reactpy.component
def App():
return reactpy.html.div(
reactpy.html.div(
reactpy.html.button(
{
"id": "the-button",
"onClick": """javascript: () => {
Copy link
Contributor

@Archmonger Archmonger Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm okay with the general direction of this approach, but I don't like that things have to be prefixed with javascript:. Would be better if we assumed all string-types are JavaScript, which would allow html_to_vdom to work properly with converted HTML. See below for an example that should work after this PR.

button_html = """<button onclick='console.log("hell world")'>Click Me</button>"""

my_vdom = html_to_vdom(button_html)

If an indicator is absolutely required, then we can automatically transform any user-provided strings to prefix javascript: to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to support that, but I couldn't come up with how. How would we differentiate an attribute from an event_handler? The separate_attributes_and_event_handlers function currently does so by checking if callable(v), and otherwise treats it as an attribute.

I thought we could check if the attribute name starts with "on", but that falls short. Really the possibilities are endless when it comes to components. I could have some property called doThisThing or whatever I wanted that expects a callable.

Copy link
Contributor

@Archmonger Archmonger Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe on<CamelCaseText> is a reserved namespace in ReactJS. Anything following that pattern is treated as an event by ReactJS, so I don't see a problem with us taking the same approach within ReactPy. In this case, we'd be analyzing the key using regex for ^on[A-Z] to detect if it is an event.

This logic could be used within separate_attributes_and_event_handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case that I'm specifically designing for has a property named getRowId that expects a callable. How do we catch for such cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.ag-grid.com/react-data-grid/row-ids/#reference-rowModels-getRowId

Copy link
Contributor

@Archmonger Archmonger Mar 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Off the top of my head, the best way of handling this would be a new data type. Perhaps it can be called JavaScriptText, which would just a plain subclass of str. The data type will strictly exist to denote a string can be run via client side eval.

class JavaScriptText(str):
    ...

String content within an ^on[A-Z] prop should automatically be transformed into JavaScriptText

pattern = re.compile(r"^on[A-Z]")

if pattern.match(key) and isinstance(value, str):
    value = JavaScriptText(value)

... but users should be allowed to manually construct them as needed.

{ "getRowId" : JavaScriptText("console.log('hello world')") }

This would allow for a deterministic way of verifying if some string is eval capable of not.

def js_eval_compatible(value):
    return isinstance(value, JavaScriptText)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Love that - I will work to get it incorporated.

let parent = document.getElementById("the-parent");
parent.appendChild(document.createElement("div"));
}""",
},
"Click Me",
),
reactpy.html.div({"id": "the-parent"}),
)
)

await display.show(lambda: App())

button = await display.page.wait_for_selector("#the-button", state="attached")
await button.click()
await button.click()
await button.click()
parent = await display.page.wait_for_selector(
"#the-parent", state="attached", timeout=0
)
generated_divs = await parent.query_selector_all("div")

assert len(generated_divs) == 3
Loading