Support raw JavaScript events

[shawncrawley]

Description

This is my WIP draft of implementing feature #1261. With my changes already in place, it works. I just wanted to start the conversation and get feedback on my approach.

Checklist

Please update this checklist as you complete each item:

• Tests have been developed for bug fixes or new functionality.
• The changelog has been updated, if necessary.
• Documentation has been updated, if necessary.
• GitHub Issues closed by this PR have been linked.

_{By submitting this pull request I agree that all contributions comply with this project's open source license(s).}

[Archmonger]

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.

[shawncrawley]

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.

[Archmonger]

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.

[shawncrawley]

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?

[shawncrawley]

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

[Archmonger]

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)

[shawncrawley]

Love that - I will work to get it incorporated.

[Archmonger]

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

[shawncrawley]

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?

[shawncrawley]

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.

[Archmonger]

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.

[shawncrawley]

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.

[Archmonger]

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.

[shawncrawley]

I implemented what we discussed. Instead of JavaScriptText as the new type, I went with simply JavaScript since it seemed more succinct and straightforward. But let me know if you have concerns there.

Also, I kept the idea of differentiating between a JavaScript and standard event handler on the client side by appending “javascript: “ to the target property of the JavaScript version of the EventHandlerType on the server side - so all black-boxed to the user.

Ultimately, I think it would make more sense to track/manage this new JavaScript case separately by modifying the Vdom schema to include a new "javascriptHandlers" alongside the existing "eventHandlers" or something. I was going to do that, but hesitated because I don't know how formalized the Vdom dict is - or if it's a thing outside of this project. That would certainly solve some of the quirkiness that I described above. My main motivation was from the quirkiness that has to be added to the react.js web template to ensure this works with web components.

Take a look and let me know what you think. If you're happy with the quirks, then this should be good to go minus the changelog being added.

[Archmonger]

The VDOM spec isn't exactly formal. We cloned the spec from a different framework (nteract), but we don't have a strong reason for keeping it identical.

I have an open issue ticket to change the VDOM spec to look cleaner on the Python side.

[Archmonger]

If you want to implement changes to the VDOM spec within this PR, I am okay with that.

[Archmonger]

Might need to change isinstance(v, str) to type(v) == str, since technically isinstance(JavaScript(), str) is True.

[Archmonger]

Add a docstring to explain that this class is a simple way of marking JavaScript code to be executed in by the browser

[Archmonger]

Another test is needed to see if string_to_reactpy works well with this implementation.

[Archmonger]

Closing this in favor of #1290