From e8231ff45eacf94d860a45aa33a065d90201715c Mon Sep 17 00:00:00 2001 From: rmorshea Date: Tue, 24 Jan 2023 20:59:22 -0800 Subject: [PATCH] begin vdom interface unification --- scripts/fix_vdom_constructor_usage.py | 361 ++++++++++++++++++ .../idom-client-react/src/element-utils.js | 23 +- src/idom/core/types.py | 11 +- src/idom/core/vdom.py | 100 ++--- src/idom/html.py | 31 +- 5 files changed, 412 insertions(+), 114 deletions(-) create mode 100644 scripts/fix_vdom_constructor_usage.py diff --git a/scripts/fix_vdom_constructor_usage.py b/scripts/fix_vdom_constructor_usage.py new file mode 100644 index 000000000..b11783d18 --- /dev/null +++ b/scripts/fix_vdom_constructor_usage.py @@ -0,0 +1,361 @@ +from __future__ import annotations + +import ast +import re +import sys +from collections.abc import Sequence +from keyword import kwlist +from pathlib import Path +from textwrap import dedent, indent +from tokenize import COMMENT as COMMENT_TOKEN +from tokenize import generate_tokens +from typing import Iterator + +from idom import html + + +_CAMEL_CASE_SUB_PATTERN = re.compile(r"(? None: + tree = ast.parse(source) + + changed: list[Sequence[ast.AST]] = [] + for parents, node in walk_with_parent(tree): + if isinstance(node, ast.Call): + func = node.func + match func: + case ast.Attribute(): + name = func.attr + case ast.Name(ctx=ast.Load()): + name = func.id + case _: + name = "" + if hasattr(html, name): + match node.args: + case [ast.Dict(keys, values), *_]: + new_kwargs = list(node.keywords) + for k, v in zip(keys, values): + if isinstance(k, ast.Constant) and isinstance(k.value, str): + new_kwargs.append( + ast.keyword(arg=conv_attr_name(k.value), value=v) + ) + else: + new_kwargs = [ast.keyword(arg=None, value=node.args[0])] + break + node.args = node.args[1:] + node.keywords = new_kwargs + changed.append((node, *parents)) + case [ + ast.Call( + func=ast.Name(id="dict", ctx=ast.Load()), + args=args, + keywords=kwargs, + ), + *_, + ]: + new_kwargs = [ + *[ast.keyword(arg=None, value=a) for a in args], + *node.keywords, + ] + for kw in kwargs: + if kw.arg is not None: + new_kwargs.append( + ast.keyword( + arg=conv_attr_name(kw.arg), value=kw.value + ) + ) + else: + new_kwargs.append(kw) + node.args = node.args[1:] + node.keywords = new_kwargs + changed.append((node, *parents)) + + case _: + pass + + if not changed: + return + + ast.fix_missing_locations(tree) + + lines = source.split("\n") + + # find closest parent nodes that should be re-written + nodes_to_unparse: list[ast.AST] = [] + for node_lineage in changed: + origin_node = node_lineage[0] + for i in range(len(node_lineage) - 1): + current_node, next_node = node_lineage[i : i + 2] + if ( + not hasattr(next_node, "lineno") + or next_node.lineno < origin_node.lineno + or isinstance(next_node, (ast.ClassDef, ast.FunctionDef)) + ): + nodes_to_unparse.append(current_node) + break + else: + raise RuntimeError("Failed to change code") + + # check if an nodes to rewrite contain eachother, pick outermost nodes + current_outermost_node, *sorted_nodes_to_unparse = list( + sorted(nodes_to_unparse, key=lambda n: n.lineno) + ) + outermost_nodes_to_unparse = [current_outermost_node] + for node in sorted_nodes_to_unparse: + if node.lineno > current_outermost_node.end_lineno: + current_outermost_node = node + outermost_nodes_to_unparse.append(node) + + moved_comment_lines_from_end: list[int] = [] + # now actually rewrite these nodes (in reverse to avoid changes earlier in file) + for node in reversed(outermost_nodes_to_unparse): + # make a best effort to preserve any comments that we're going to overwrite + comments = find_comments(lines[node.lineno - 1 : node.end_lineno]) + + # there may be some content just before and after the content we're re-writing + before_replacement = lines[node.lineno - 1][: node.col_offset].strip() + + if node.end_lineno is not None and node.end_col_offset is not None: + after_replacement = lines[node.end_lineno - 1][ + node.end_col_offset : + ].strip() + else: + after_replacement = "" + + replacement = indent( + before_replacement + + "\n".join([*comments, ast.unparse(node)]) + + after_replacement, + " " * (node.col_offset - len(before_replacement)), + ) + + if node.end_lineno: + lines[node.lineno - 1 : node.end_lineno] = [replacement] + else: + lines[node.lineno - 1] = replacement + + if comments: + moved_comment_lines_from_end.append(len(lines) - node.lineno) + + for lineno_from_end in sorted(list(set(moved_comment_lines_from_end))): + print(f"Moved comments to {filename}:{len(lines) - lineno_from_end}") + + return "\n".join(lines) + + +def find_comments(lines: list[str]) -> list[str]: + iter_lines = iter(lines) + return [ + token + for token_type, token, _, _, _ in generate_tokens(lambda: next(iter_lines)) + if token_type == COMMENT_TOKEN + ] + + +def walk_with_parent( + node: ast.AST, parents: tuple[ast.AST, ...] = () +) -> Iterator[tuple[tuple[ast.AST, ...], ast.AST]]: + parents = (node,) + parents + for child in ast.iter_child_nodes(node): + yield parents, child + yield from walk_with_parent(child, parents) + + +def conv_attr_name(name: str) -> str: + new_name = _CAMEL_CASE_SUB_PATTERN.sub("_", name).replace("-", "_").lower() + return f"{new_name}_" if new_name in kwlist else new_name + + +def run_tests(): + cases = [ + # simple conversions + ( + 'html.div({"className": "test"})', + "html.div(class_name='test')", + ), + ( + 'html.div({class_name: "test", **other})', + "html.div(**{class_name: 'test', **other})", + ), + ( + 'html.div(dict(other, className="test"))', + "html.div(**other, class_name='test')", + ), + ( + 'html.div({"className": "outer"}, html.div({"className": "inner"}))', + "html.div(html.div(class_name='inner'), class_name='outer')", + ), + ( + 'html.div({"className": "outer"}, html.div({"className": "inner"}))', + "html.div(html.div(class_name='inner'), class_name='outer')", + ), + ( + '["before", html.div({"className": "test"}), "after"]', + "['before', html.div(class_name='test'), 'after']", + ), + ( + """ + html.div( + {"className": "outer"}, + html.div({"className": "inner"}), + html.div({"className": "inner"}), + ) + """, + "html.div(html.div(class_name='inner'), html.div(class_name='inner'), class_name='outer')", + ), + ( + 'html.div(dict(className="test"))', + "html.div(class_name='test')", + ), + # when to not attempt conversion + ( + 'html.div(ignore, {"className": "test"})', + None, + ), + # avoid unnecessary changes + ( + """ + def my_function(): + x = 1 # some comment + return html.div({"className": "test"}) + """, + """ + def my_function(): + x = 1 # some comment + return html.div(class_name='test') + """, + ), + ( + """ + if condition: + # some comment + dom = html.div({"className": "test"}) + """, + """ + if condition: + # some comment + dom = html.div(class_name='test') + """, + ), + ( + """ + [ + html.div({"className": "test"}), + html.div({"className": "test"}), + ] + """, + """ + [ + html.div(class_name='test'), + html.div(class_name='test'), + ] + """, + ), + ( + """ + @deco( + html.div({"className": "test"}), + html.div({"className": "test"}), + ) + def func(): + # comment + x = [ + 1 + ] + """, + """ + @deco( + html.div(class_name='test'), + html.div(class_name='test'), + ) + def func(): + # comment + x = [ + 1 + ] + """, + ), + ( + """ + @deco(html.div({"className": "test"}), html.div({"className": "test"})) + def func(): + # comment + x = [ + 1 + ] + """, + """ + @deco(html.div(class_name='test'), html.div(class_name='test')) + def func(): + # comment + x = [ + 1 + ] + """, + ), + # best effort to preserve comments + ( + """ + x = 1 + html.div( + # comment 1 + {"className": "outer"}, + # comment 2 + html.div({"className": "inner"}), + ) + """, + """ + x = 1 + # comment 1 + # comment 2 + html.div(html.div(class_name='inner'), class_name='outer') + """, + ), + ] + + success = True + + for source, expected in cases: + actual = update_vdom_constructor_usages(dedent(source).strip(), "test.py") + if isinstance(expected, str): + expected = dedent(expected).strip() + if actual != expected: + if not success: + print("\n" + "-" * 20) + print( + dedent( + f""" + +{actual} + +▲ actual ▲ +▼ expected ▼ + +{expected} + + """ + ) + ) + success = False + + return success + + +if __name__ == "__main__": + argv = sys.argv[1:] + + if not argv: + print("Running tests...") + result = run_tests() + print("Success" if result else "Failed") + sys.exit(0 if result else 0) + + for pattern in argv: + for file in Path.cwd().glob(pattern): + result = update_vdom_constructor_usages( + source=file.read_text(), + filename=str(file), + ) + if result is not None: + file.write_text(result) diff --git a/src/client/packages/idom-client-react/src/element-utils.js b/src/client/packages/idom-client-react/src/element-utils.js index 2300d6d8b..afeb7a62e 100644 --- a/src/client/packages/idom-client-react/src/element-utils.js +++ b/src/client/packages/idom-client-react/src/element-utils.js @@ -22,18 +22,16 @@ export function createElementAttributes(model, sendEvent) { if (model.eventHandlers) { for (const [eventName, eventSpec] of Object.entries(model.eventHandlers)) { - attributes[eventName] = createEventHandler( - eventName, - sendEvent, - eventSpec - ); + attributes[eventName] = createEventHandler(sendEvent, eventSpec); } } - return attributes; + return Object.fromEntries( + Object.entries(attributes).map(([key, value]) => [snakeToCamel(key), value]) + ); } -function createEventHandler(eventName, sendEvent, eventSpec) { +function createEventHandler(sendEvent, eventSpec) { return function () { const data = Array.from(arguments).map((value) => { if (typeof value === "object" && value.nativeEvent) { @@ -51,7 +49,16 @@ function createEventHandler(eventName, sendEvent, eventSpec) { sendEvent({ data: data, target: eventSpec["target"], - type: "layout-event", }); }; } + +function snakeToCamel(str) { + if (str.startsWith("data_") || str.startsWith("aria_")) { + return str.replace("_", "-"); + } else { + return str + .toLowerCase() + .replace(/([-_][a-z])/g, (group) => group.toUpperCase().replace("_", "")); + } +} diff --git a/src/idom/core/types.py b/src/idom/core/types.py index 0fd78ec22..0beab45ec 100644 --- a/src/idom/core/types.py +++ b/src/idom/core/types.py @@ -62,10 +62,13 @@ class ComponentType(Protocol): This is used to see if two component instances share the same definition. """ - def render(self) -> VdomDict | ComponentType | str | None: + def render(self) -> RenderResult: """Render the component's view model.""" +RenderResult = Union["VdomDict", ComponentType, str, None] + + _Render = TypeVar("_Render", covariant=True) _Event = TypeVar("_Event", contravariant=True) @@ -208,9 +211,9 @@ class VdomDictConstructor(Protocol): def __call__( self, - *attributes_and_children: VdomAttributesAndChildren, - key: str = ..., - event_handlers: Optional[EventHandlerMapping] = ..., + *children: VdomChild, + key: Key | None = None, + **attributes: Any, ) -> VdomDict: ... diff --git a/src/idom/core/vdom.py b/src/idom/core/vdom.py index 78bfb3725..eb10da890 100644 --- a/src/idom/core/vdom.py +++ b/src/idom/core/vdom.py @@ -1,10 +1,9 @@ from __future__ import annotations import logging -from typing import Any, Dict, List, Mapping, Optional, Sequence, Tuple, cast +from typing import Any, Mapping, cast from fastjsonschema import compile as compile_json_schema -from typing_extensions import Protocol from idom.config import IDOM_DEBUG_MODE from idom.core.events import ( @@ -15,11 +14,12 @@ from idom.core.types import ( ComponentType, EventHandlerDict, - EventHandlerMapping, EventHandlerType, ImportSourceDict, - VdomAttributesAndChildren, + Key, + VdomChild, VdomDict, + VdomDictConstructor, VdomJson, ) @@ -129,13 +129,9 @@ def is_vdom(value: Any) -> bool: def vdom( - tag: str, - *attributes_and_children: VdomAttributesAndChildren, - key: str | int | None = None, - event_handlers: Optional[EventHandlerMapping] = None, - import_source: Optional[ImportSourceDict] = None, + tag: str, *children: VdomChild, key: Key | None = None, **attributes: Any ) -> VdomDict: - """A helper function for creating VDOM dictionaries. + """A helper function for creating VDOM elements. Parameters: tag: @@ -157,10 +153,14 @@ def vdom( """ model: VdomDict = {"tagName": tag} - attributes, children = coalesce_attributes_and_children(attributes_and_children) - attributes, event_handlers = separate_attributes_and_event_handlers( - attributes, event_handlers or {} - ) + children: list[VdomChild] = [] + for child in children: + if _is_single_child(child): + children.append(child) + else: + children.extend(child) + + attributes, event_handlers = separate_attributes_and_event_handlers(attributes) if attributes: model["attributes"] = attributes @@ -174,26 +174,14 @@ def vdom( if key is not None: model["key"] = key - if import_source is not None: - model["importSource"] = import_source - return model -class _VdomDictConstructor(Protocol): - def __call__( - self, - *attributes_and_children: VdomAttributesAndChildren, - key: str | int | None = ..., - event_handlers: Optional[EventHandlerMapping] = ..., - import_source: Optional[ImportSourceDict] = ..., - ) -> VdomDict: - ... +def with_import_source(element: VdomDict, import_source: ImportSourceDict) -> VdomDict: + return {**element, "importSource": import_source} -def make_vdom_constructor( - tag: str, allow_children: bool = True -) -> _VdomDictConstructor: +def make_vdom_constructor(tag: str, allow_children: bool = True) -> VdomDictConstructor: """Return a constructor for VDOM dictionaries with the given tag name. The resulting callable will have the same interface as :func:`vdom` but without its @@ -201,21 +189,11 @@ def make_vdom_constructor( """ def constructor( - *attributes_and_children: VdomAttributesAndChildren, - key: str | int | None = None, - event_handlers: Optional[EventHandlerMapping] = None, - import_source: Optional[ImportSourceDict] = None, + *children: VdomChild, key: Key | None = None, **attributes: Any ) -> VdomDict: - model = vdom( - tag, - *attributes_and_children, - key=key, - event_handlers=event_handlers, - import_source=import_source, - ) - if not allow_children and "children" in model: + if not allow_children and children: raise TypeError(f"{tag!r} nodes cannot have children.") - return model + return vdom(tag, *children, key=key, **attributes) # replicate common function attributes constructor.__name__ = tag @@ -233,36 +211,11 @@ def constructor( return constructor -def coalesce_attributes_and_children( - values: Sequence[Any], -) -> Tuple[Mapping[str, Any], List[Any]]: - if not values: - return {}, [] - - children_or_iterables: Sequence[Any] - attributes, *children_or_iterables = values - if not _is_attributes(attributes): - attributes = {} - children_or_iterables = values - - children: List[Any] = [] - for child in children_or_iterables: - if _is_single_child(child): - children.append(child) - else: - children.extend(child) - - return attributes, children - - def separate_attributes_and_event_handlers( - attributes: Mapping[str, Any], event_handlers: EventHandlerMapping -) -> Tuple[Dict[str, Any], EventHandlerDict]: + attributes: Mapping[str, Any] +) -> tuple[dict[str, Any], EventHandlerDict]: separated_attributes = {} - separated_event_handlers: Dict[str, List[EventHandlerType]] = {} - - for k, v in event_handlers.items(): - separated_event_handlers[k] = [v] + separated_event_handlers: dict[str, list[EventHandlerType]] = {} for k, v in attributes.items(): @@ -271,7 +224,8 @@ def separate_attributes_and_event_handlers( if callable(v): handler = EventHandler(to_event_handler_function(v)) elif ( - # isinstance check on protocols is slow, function attr check is a quick filter + # isinstance check on protocols is slow - use function attr pre-check as a + # quick filter before actually performing slow EventHandlerType type check hasattr(v, "function") and isinstance(v, EventHandlerType) ): @@ -292,10 +246,6 @@ def separate_attributes_and_event_handlers( return separated_attributes, flat_event_handlers_dict -def _is_attributes(value: Any) -> bool: - return isinstance(value, Mapping) and "tagName" not in value - - def _is_single_child(value: Any) -> bool: if isinstance(value, (str, Mapping)) or not hasattr(value, "__iter__"): return True diff --git a/src/idom/html.py b/src/idom/html.py index 964af5d6e..a2174063a 100644 --- a/src/idom/html.py +++ b/src/idom/html.py @@ -160,7 +160,7 @@ from typing import Any, Mapping from idom.core.types import Key, VdomDict -from idom.core.vdom import coalesce_attributes_and_children, make_vdom_constructor +from idom.core.vdom import make_vdom_constructor, vdom __all__ = ( @@ -280,18 +280,7 @@ def _(*children: Any, key: Key | None = None) -> VdomDict: """An HTML fragment - this element will not appear in the DOM""" - attributes, coalesced_children = coalesce_attributes_and_children(children) - if attributes: - raise TypeError("Fragments cannot have attributes") - model: VdomDict = {"tagName": ""} - - if coalesced_children: - model["children"] = coalesced_children - - if key is not None: - model["key"] = key - - return model + return vdom("", *children, key=key) # Dcument metadata @@ -389,10 +378,7 @@ def _(*children: Any, key: Key | None = None) -> VdomDict: noscript = make_vdom_constructor("noscript") -def script( - *attributes_and_children: Mapping[str, Any] | str, - key: str | int | None = None, -) -> VdomDict: +def script(*children: str, key: Key | None = None, **attributes: Any) -> VdomDict: """Create a new `<{script}> `__ element. This behaves slightly differently than a normal script element in that it may be run @@ -406,29 +392,20 @@ def script( function that is called when the script element is removed from the tree, or when the script content changes. """ - model: VdomDict = {"tagName": "script"} - - attributes, children = coalesce_attributes_and_children(attributes_and_children) - if children: if len(children) > 1: raise ValueError("'script' nodes may have, at most, one child.") elif not isinstance(children[0], str): raise ValueError("The child of a 'script' must be a string.") else: - model["children"] = children if key is None: key = children[0] if attributes: - model["attributes"] = attributes if key is None and not children and "src" in attributes: key = attributes["src"] - if key is not None: - model["key"] = key - - return model + return vdom("script", *children, key=key, **attributes) # Demarcating edits