From 0a5df7b2ea478ab737cf68cb9946f2dca8180bfd Mon Sep 17 00:00:00 2001 From: Ryan Morshead Date: Mon, 3 Jul 2023 14:04:36 -0600 Subject: [PATCH 1/5] identify issue from #1081 --- src/py/reactpy/tests/test_core/test_layout.py | 49 ++++++++ src/py/reactpy/tests/tooling/layout.py | 37 ++++++ src/py/reactpy/tests/tooling/select.py | 107 ++++++++++++++++++ 3 files changed, 193 insertions(+) create mode 100644 src/py/reactpy/tests/tooling/layout.py create mode 100644 src/py/reactpy/tests/tooling/select.py diff --git a/src/py/reactpy/tests/test_core/test_layout.py b/src/py/reactpy/tests/test_core/test_layout.py index d2e1a8099..c27a05167 100644 --- a/src/py/reactpy/tests/test_core/test_layout.py +++ b/src/py/reactpy/tests/test_core/test_layout.py @@ -13,6 +13,7 @@ from reactpy.core.component import component from reactpy.core.hooks import use_effect, use_state from reactpy.core.layout import Layout +from reactpy.core.types import State from reactpy.testing import ( HookCatcher, StaticEventHandler, @@ -20,8 +21,11 @@ capture_reactpy_logs, ) from reactpy.utils import Ref +from tests.tooling import select from tests.tooling.common import event_message, update_message from tests.tooling.hooks import use_force_render, use_toggle +from tests.tooling.layout import layout_runner +from tests.tooling.select import element_exists, find_element @pytest.fixture(autouse=True) @@ -1190,3 +1194,48 @@ def Child(): done, pending = await asyncio.wait([render_task], timeout=0.1) assert not done and pending render_task.cancel() + + +async def test_something(): + @component + def Item(item: str, all_items: State[list[str]]): + color = use_state(None) + + def deleteme(event): + all_items.set_value([i for i in all_items.value if (i != item)]) + + def colorize(event): + color.set_value("blue" if not color.value else None) + + return html.div( + {"id": item, "color": color.value}, + html.button({"on_click": colorize}, f"Color {item}"), + html.button({"on_click": deleteme}, f"Delete {item}"), + ) + + @component + def App(): + items = use_state(["A", "B", "C"]) + return html._([Item(item, items, key=item) for item in items.value]) + + async with layout_runner(reactpy.Layout(App())) as runner: + # Delete item B + tree = await runner.render() + b, b_info = find_element(tree, select.id_equals("B")) + assert b_info.path == (0, 1, 0) + b_delete, _ = find_element(b, select.text_equals("Delete B")) + await runner.trigger(b_delete, "on_click", {}) + + # Set color of item C + tree = await runner.render() + assert not element_exists(tree, select.id_equals("B")) + c, c_info = find_element(tree, select.id_equals("C")) + assert c_info.path == (0, 1, 0) + c_color, _ = find_element(c, select.text_equals("Color C")) + await runner.trigger(c_color, "on_click", {}) + + # Ensure position and color of item C are correct + tree = await runner.render() + c, c_info = find_element(tree, select.id_equals("C")) + assert c_info.path == (0, 1, 0) + assert c["attributes"]["color"] == "blue" diff --git a/src/py/reactpy/tests/tooling/layout.py b/src/py/reactpy/tests/tooling/layout.py new file mode 100644 index 000000000..259a00be9 --- /dev/null +++ b/src/py/reactpy/tests/tooling/layout.py @@ -0,0 +1,37 @@ +from __future__ import annotations + +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager +from typing import Any + +from jsonpointer import set_pointer + +from reactpy.core.layout import Layout +from reactpy.core.types import VdomJson +from tests.tooling.common import event_message + + +@asynccontextmanager +async def layout_runner(layout: Layout) -> AsyncIterator[LayoutRunner]: + async with layout: + yield LayoutRunner(layout) + + +class LayoutRunner: + def __init__(self, layout: Layout) -> None: + self.layout = layout + self.model = {} + + async def render(self) -> VdomJson: + update = await self.layout.render() + if not update["path"]: + self.model = update["model"] + else: + self.model = set_pointer(self.model, update["path"], update["model"]) + return self.model + + async def trigger(self, element: VdomJson, event_name: str, *data: Any) -> None: + event_handler = element.get("eventHandlers", {}).get(event_name, {}) + if not event_handler: + raise ValueError(f"Element has no event handler for {event_name}") + await self.layout.deliver(event_message(event_handler["target"], *data)) diff --git a/src/py/reactpy/tests/tooling/select.py b/src/py/reactpy/tests/tooling/select.py new file mode 100644 index 000000000..cf7a9c004 --- /dev/null +++ b/src/py/reactpy/tests/tooling/select.py @@ -0,0 +1,107 @@ +from __future__ import annotations + +from collections.abc import Iterator, Sequence +from dataclasses import dataclass +from typing import Callable + +from reactpy.core.types import VdomJson + +Selector = Callable[[VdomJson, "ElementInfo"], bool] + + +def id_equals(id: str) -> Selector: + return lambda element, _: element.get("attributes", {}).get("id") == id + + +def class_equals(class_name: str) -> Selector: + return ( + lambda element, _: class_name + in element.get("attributes", {}).get("class", "").split() + ) + + +def text_equals(text: str) -> Selector: + return lambda element, _: _element_text(element) == text + + +def _element_text(element: VdomJson) -> str: + if isinstance(element, str): + return element + return "".join(_element_text(child) for child in element.get("children", [])) + + +def element_exists(element: VdomJson, selector: Selector) -> bool: + return next(find_elements(element, selector), None) is not None + + +def find_element( + element: VdomJson, + selector: Selector, + *, + first: bool = False, +) -> tuple[VdomJson, ElementInfo]: + """Find an element by a selector. + + Parameters: + element: + The tree to search. + selector: + A function that returns True if the element matches. + first: + If True, return the first element found. If False, raise an error if + multiple elements are found. + + Returns: + Element info, or None if not found. + """ + find_iter = find_elements(element, selector) + found = next(find_iter, None) + if found is None: + raise ValueError("Element not found") + if not first: + try: + next(find_iter) + raise ValueError("Multiple elements found") + except StopIteration: + pass + return found + + +def find_elements( + element: VdomJson, selector: Selector +) -> Iterator[tuple[VdomJson, ElementInfo]]: + """Find an element by a selector. + + Parameters: + element: + The tree to search. + selector: + A function that returns True if the element matches. + + Returns: + Element info, or None if not found. + """ + return _find_elements(element, selector, (), ()) + + +def _find_elements( + element: VdomJson, + selector: Selector, + parents: Sequence[VdomJson], + path: Sequence[int], +) -> tuple[VdomJson, ElementInfo] | None: + info = ElementInfo(parents, path) + if selector(element, info): + yield element, info + + for index, child in enumerate(element.get("children", [])): + if isinstance(child, dict): + yield from _find_elements( + child, selector, (*parents, element), (*path, index) + ) + + +@dataclass +class ElementInfo: + parents: Sequence[VdomJson] + path: Sequence[int] From 75881a5671239e149713be55dd79b4ab64037955 Mon Sep 17 00:00:00 2001 From: Ryan Morshead Date: Mon, 3 Jul 2023 19:12:40 -0600 Subject: [PATCH 2/5] fix the bug --- src/py/reactpy/pyproject.toml | 1 + src/py/reactpy/reactpy/core/layout.py | 2 +- src/py/reactpy/tests/test_core/test_layout.py | 17 +++++++++++++---- src/py/reactpy/tests/tooling/layout.py | 9 ++++++++- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/py/reactpy/pyproject.toml b/src/py/reactpy/pyproject.toml index 659ddbf94..87fa7e036 100644 --- a/src/py/reactpy/pyproject.toml +++ b/src/py/reactpy/pyproject.toml @@ -139,6 +139,7 @@ testpaths = "tests" xfail_strict = true python_files = "*asserts.py test_*.py" asyncio_mode = "auto" +log_cli_level = "INFO" # --- MyPy ----------------------------------------------------------------------------- diff --git a/src/py/reactpy/reactpy/core/layout.py b/src/py/reactpy/reactpy/core/layout.py index df24a9a0a..f84cb104e 100644 --- a/src/py/reactpy/reactpy/core/layout.py +++ b/src/py/reactpy/reactpy/core/layout.py @@ -489,7 +489,7 @@ def _update_component_model_state( index=new_index, key=old_model_state.key, model=Ref(), # does not copy the model - patch_path=old_model_state.patch_path, + patch_path=f"{new_parent.patch_path}/children/{new_index}", children_by_key={}, targets_by_event={}, life_cycle_state=( diff --git a/src/py/reactpy/tests/test_core/test_layout.py b/src/py/reactpy/tests/test_core/test_layout.py index c27a05167..ae0d6c4b5 100644 --- a/src/py/reactpy/tests/test_core/test_layout.py +++ b/src/py/reactpy/tests/test_core/test_layout.py @@ -1196,7 +1196,13 @@ def Child(): render_task.cancel() -async def test_something(): +async def test_ensure_model_path_is_updated_on_update(): + """ + This is regression test for a bug in which we failed to update the path of a bug + that arose when the "path" of a component within the overall model was not updated + when the component changes position amongst its siblings. + """ + @component def Item(item: str, all_items: State[list[str]]): color = use_state(None) @@ -1219,23 +1225,26 @@ def App(): return html._([Item(item, items, key=item) for item in items.value]) async with layout_runner(reactpy.Layout(App())) as runner: - # Delete item B tree = await runner.render() + + # Delete item B b, b_info = find_element(tree, select.id_equals("B")) assert b_info.path == (0, 1, 0) b_delete, _ = find_element(b, select.text_equals("Delete B")) await runner.trigger(b_delete, "on_click", {}) - # Set color of item C tree = await runner.render() + + # Set color of item C assert not element_exists(tree, select.id_equals("B")) c, c_info = find_element(tree, select.id_equals("C")) assert c_info.path == (0, 1, 0) c_color, _ = find_element(c, select.text_equals("Color C")) await runner.trigger(c_color, "on_click", {}) - # Ensure position and color of item C are correct tree = await runner.render() + + # Ensure position and color of item C are correct c, c_info = find_element(tree, select.id_equals("C")) assert c_info.path == (0, 1, 0) assert c["attributes"]["color"] == "blue" diff --git a/src/py/reactpy/tests/tooling/layout.py b/src/py/reactpy/tests/tooling/layout.py index 259a00be9..fe78684fe 100644 --- a/src/py/reactpy/tests/tooling/layout.py +++ b/src/py/reactpy/tests/tooling/layout.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging from collections.abc import AsyncIterator from contextlib import asynccontextmanager from typing import Any @@ -10,6 +11,8 @@ from reactpy.core.types import VdomJson from tests.tooling.common import event_message +logger = logging.getLogger(__name__) + @asynccontextmanager async def layout_runner(layout: Layout) -> AsyncIterator[LayoutRunner]: @@ -24,14 +27,18 @@ def __init__(self, layout: Layout) -> None: async def render(self) -> VdomJson: update = await self.layout.render() + logger.info(f"Rendering element at {update['path'] or '/'!r}") if not update["path"]: self.model = update["model"] else: - self.model = set_pointer(self.model, update["path"], update["model"]) + self.model = set_pointer( + self.model, update["path"], update["model"], inplace=False + ) return self.model async def trigger(self, element: VdomJson, event_name: str, *data: Any) -> None: event_handler = element.get("eventHandlers", {}).get(event_name, {}) + logger.info(f"Triggering {event_name!r} with target {event_handler['target']}") if not event_handler: raise ValueError(f"Element has no event handler for {event_name}") await self.layout.deliver(event_message(event_handler["target"], *data)) From 121601fc1eda6b6e1d2daade46d3afe6de339efe Mon Sep 17 00:00:00 2001 From: Ryan Morshead Date: Mon, 3 Jul 2023 19:30:00 -0600 Subject: [PATCH 3/5] update doc --- src/py/reactpy/tests/test_core/test_layout.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/py/reactpy/tests/test_core/test_layout.py b/src/py/reactpy/tests/test_core/test_layout.py index ae0d6c4b5..215e89137 100644 --- a/src/py/reactpy/tests/test_core/test_layout.py +++ b/src/py/reactpy/tests/test_core/test_layout.py @@ -1196,11 +1196,13 @@ def Child(): render_task.cancel() -async def test_ensure_model_path_is_updated_on_update(): +async def test_ensure_model_path_udpates(): """ This is regression test for a bug in which we failed to update the path of a bug that arose when the "path" of a component within the overall model was not updated - when the component changes position amongst its siblings. + when the component changes position amongst its siblings. This meant that when + a component whose position had changed would attempt to update the view at its old + position. """ @component From 02b1995098a4a6ab28485247554a6010905660ba Mon Sep 17 00:00:00 2001 From: Ryan Morshead Date: Mon, 3 Jul 2023 19:32:44 -0600 Subject: [PATCH 4/5] make ruff happy --- src/py/reactpy/reactpy/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/py/reactpy/reactpy/utils.py b/src/py/reactpy/reactpy/utils.py index e5e06d98d..5624846a4 100644 --- a/src/py/reactpy/reactpy/utils.py +++ b/src/py/reactpy/reactpy/utils.py @@ -27,7 +27,7 @@ class Ref(Generic[_RefValue]): You can compare the contents for two ``Ref`` objects using the ``==`` operator. """ - __slots__ = "current" + __slots__ = ("current",) def __init__(self, initial_value: _RefValue = _UNDEFINED) -> None: if initial_value is not _UNDEFINED: From 0e4ce5dfd50d029ed4a983511d8465a555dc3260 Mon Sep 17 00:00:00 2001 From: Ryan Morshead Date: Mon, 3 Jul 2023 20:30:26 -0600 Subject: [PATCH 5/5] add changelog entry --- docs/source/about/changelog.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/source/about/changelog.rst b/docs/source/about/changelog.rst index a6eff8f73..a927f0fcf 100644 --- a/docs/source/about/changelog.rst +++ b/docs/source/about/changelog.rst @@ -31,6 +31,7 @@ Unreleased - :issue:`930` - better traceback for JSON serialization errors (via :pull:`1008`) - :issue:`437` - explain that JS component attributes must be JSON (via :pull:`1008`) +- :issue:`1086` - fix rendering bug when children change positions (via :pull:`1085`) v1.0.0