Support inline JavaScript events (v2) #1290
Conversation
|
This is ready to be looked at - again, aside from the changelog. I can get this to work "out of the box" with I ultimately assumed that coercing them to camelCase on the Vdom side is unnecessary overhead. Let me know if you agree. |
|
|
Co-authored-by: Mark Bakhit <[email protected]>
|
All comments have been addressed, changelog is updated... This PR should be ready to go. |
| ([name, inlineJavaScript]) => | ||
| createInlineJavascript(name, inlineJavaScript), |
There was a problem hiding this comment.
Minor nitpick about capitalization consistency. Either choose Javascript or JavaScript. This same sentiment applies to every use of javascript within this PR.
There was a problem hiding this comment.
Good catch - I didn't notice I had been inconsistent myself. I'll go with JavaScript.
| def html_props_to_reactjs(vdom: VdomDict) -> None: | ||
| """Convert HTML prop names to their ReactJS equivalents.""" | ||
| if "attributes" in vdom: | ||
| items = cast(VdomAttributes, vdom["attributes"].items()) | ||
| vdom["attributes"] = cast( | ||
| VdomAttributes, | ||
| {REACT_PROP_SUBSTITUTIONS.get(k, k): v for k, v in items}, | ||
| ) | ||
| def _attributes_to_reactjs(attributes: VdomAttributes): | ||
| """Convert HTML attribute names to their ReactJS equivalents. | ||
| This method is private because it is called prior to instantiating a | ||
| Vdom class from a parsed html string, so it does not need to be called | ||
| as part of this class's instantiation (see comments in __init__ above). | ||
| """ | ||
| attrs = cast(VdomAttributes, attributes.items()) | ||
| attrs = cast( | ||
| VdomAttributes, | ||
| {REACT_PROP_SUBSTITUTIONS.get(k, k): v for k, v in attrs}, | ||
| ) | ||
| return attrs |
There was a problem hiding this comment.
Revert this function and create a separate transform function within RequiredTransforms that performs the InlineJavascript transforms.
The cadence set within RequiredTransforms is that it never needs any external calls, it just "does everything" automatically during initialization.
There was a problem hiding this comment.
I'm not sure what to do here. I know about the RequiredTransforms cadence due to the comment in its __init__ function, but to handle the "onclick" vs "onClick", "attributes" vs "inlineJavaScript", and ^on\w+ vs ^on[A-Z]\w+ dilemma (which we decided to stick with camelCase regex), I have to convert the inline html attributes to camelCase prior to instantiating the Vdom object, which is also prior to calling the RequiredTransforms - since these transforms all operate on Vdom object. Note the call order below:
I injected the camelCase transform to occur on the parsed HTML etree object (again, prior to it being used to instantiate the Vdom object and applying the standard RequiredTransforms. And if conversion to camelCase happens prior to Vdom instantiation, then there's no need to execute it again after. That is why I deleted the old function and just created a new "private" one, where it's only private so that it won't be invoked within the official cadence.
Does this makes sense? So again, I'm a bit confused. Do you want me to just restore the old function in addition to keeping the new one while maintaining the call order I've implemented? In which case it will perform a camelCase conversion twice - once before Vdom and once after.
| const wrappedExecutable = function (...args: any[]) { | ||
| function handleExecution(...args: any[]) { | ||
| const evalResult = eval(inlineJavaScript); | ||
| if (typeof evalResult == "function") { | ||
| return evalResult(...args); | ||
| } | ||
| } | ||
| if (args.length > 0 && args[0] instanceof Event) { | ||
| return handleExecution.call(args[0].currentTarget, ...args); | ||
| } else { | ||
| return handleExecution(...args); | ||
| } | ||
| }; | ||
| wrappedExecutable.isHandler = false; | ||
| return [name, wrappedExecutable]; |
There was a problem hiding this comment.
Since there's a couple of logical paths here, some comments would be appreciated on this function
| class JavaScript(str): | ||
| """Simple subclass that flags a user's string in ReactPy VDOM attributes as executable JavaScript.""" | ||
|
|
||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
Should rename this to InlineJavaScript for consistency. Would also help users understand that JavaScript is not an "executor function", and as such the following scenario is not valid syntax:
@component
def my_example():
def on_click(event):
result = JavaScript("this.innerText = 'hello'")
...
Archmonger
changed the title
Archmonger
changed the title
Description
This is an alternative solution to PR #1289, so only one of them need be merged. I created this separately to better compare approaches. This approach modifies the VDOM schema to include a new
jsExecutablesproperty alongsideeventHandlers.Checklist
Please update this checklist as you complete each item:
By submitting this pull request I agree that all contributions comply with this project's open source license(s).