remove should render

also adds a regression test in the case a component does not render its
children

Author: rmorshea

src/idom/core/component.py

@@ -54,9 +54,6 @@ def __init__(
     def render(self) -> ComponentType | VdomDict | str | None:
         return self.type(*self._args, **self._kwargs)
 
-    def should_render(self, new: Component) -> bool:
-        return True
-
     def __repr__(self) -> str:
         try:
             args = self._sig.bind(*self._args, **self._kwargs).arguments

src/idom/core/hooks.py

@@ -268,13 +268,6 @@ def use_context(context: Context[_Type]) -> _Type:
         # then we can safely access the context's default value
         return cast(_Type, context.__kwdefaults__["value"])
 
-    subscribers = provider._subscribers
-
-    @use_effect
-    def subscribe_to_context_change() -> Callable[[], None]:
-        subscribers.add(hook)
-        return lambda: subscribers.remove(hook)
-
     return provider._value
 
 
@@ -289,21 +282,12 @@ def __init__(
         self.children = children
         self.key = key
         self.type = type
-        self._subscribers: set[LifeCycleHook] = set()
         self._value = value
 
     def render(self) -> VdomDict:
         current_hook().set_context_provider(self)
         return vdom("", *self.children)
 
-    def should_render(self, new: ContextProvider[_Type]) -> bool:
-        if not strictly_equal(self._value, new._value):
-            for hook in self._subscribers:
-                hook.set_context_provider(new)
-                hook.schedule_render()
-            return True
-        return False
-
     def __repr__(self) -> str:
         return f"{type(self).__name__}({self.type})"
 

src/idom/core/layout.py

@@ -168,44 +168,35 @@ def _render_component(
         component: ComponentType,
     ) -> None:
         life_cycle_state = new_state.life_cycle_state
+        life_cycle_hook = life_cycle_state.hook
+
         self._model_states_by_life_cycle_state_id[life_cycle_state.id] = new_state
 
-        if (
-            old_state is not None
-            and hasattr(old_state.model, "current")
-            and old_state.is_component_state
-            and not _check_should_render(
-                old_state.life_cycle_state.component, component
-            )
-        ):
-            new_state.model.current = old_state.model.current
-        else:
-            life_cycle_hook = life_cycle_state.hook
-            life_cycle_hook.affect_component_will_render(component)
-            exit_stack.callback(life_cycle_hook.affect_layout_did_render)
-            life_cycle_hook.set_current()
-            try:
-                raw_model = component.render()
-                # wrap the model in a fragment (i.e. tagName="") to ensure components have
-                # a separate node in the model state tree. This could be removed if this
-                # components are given a node in the tree some other way
-                wrapper_model: VdomDict = {"tagName": ""}
-                if raw_model is not None:
-                    wrapper_model["children"] = [raw_model]
-                self._render_model(exit_stack, old_state, new_state, wrapper_model)
-            except Exception as error:
-                logger.exception(f"Failed to render {component}")
-                new_state.model.current = {
-                    "tagName": "",
-                    "error": (
-                        f"{type(error).__name__}: {error}"
-                        if IDOM_DEBUG_MODE.current
-                        else ""
-                    ),
-                }
-            finally:
-                life_cycle_hook.unset_current()
-                life_cycle_hook.affect_component_did_render()
+        life_cycle_hook.affect_component_will_render(component)
+        exit_stack.callback(life_cycle_hook.affect_layout_did_render)
+        life_cycle_hook.set_current()
+        try:
+            raw_model = component.render()
+            # wrap the model in a fragment (i.e. tagName="") to ensure components have
+            # a separate node in the model state tree. This could be removed if this
+            # components are given a node in the tree some other way
+            wrapper_model: VdomDict = {"tagName": ""}
+            if raw_model is not None:
+                wrapper_model["children"] = [raw_model]
+            self._render_model(exit_stack, old_state, new_state, wrapper_model)
+        except Exception as error:
+            logger.exception(f"Failed to render {component}")
+            new_state.model.current = {
+                "tagName": "",
+                "error": (
+                    f"{type(error).__name__}: {error}"
+                    if IDOM_DEBUG_MODE.current
+                    else ""
+                ),
+            }
+        finally:
+            life_cycle_hook.unset_current()
+            life_cycle_hook.affect_component_did_render()
 
         try:
             parent = new_state.parent
@@ -464,14 +455,6 @@ def __repr__(self) -> str:
         return f"{type(self).__name__}({self.root})"
 
 
-def _check_should_render(old: ComponentType, new: ComponentType) -> bool:
-    try:
-        return old.should_render(new)
-    except Exception:
-        logger.exception(f"{old} component failed to check if {new} should be rendered")
-        return False
-
-
 def _new_root_model_state(
     component: ComponentType, schedule_render: Callable[[_LifeCycleStateId], None]
 ) -> _ModelState:

src/idom/core/types.py

@@ -65,9 +65,6 @@ class ComponentType(Protocol):
     def render(self) -> VdomDict | ComponentType | str | None:
         """Render the component's view model."""
 
-    def should_render(self: _OwnType, new: _OwnType) -> bool:
-        """Whether the new component instance should be rendered."""
-
 
 _Render = TypeVar("_Render", covariant=True)
 _Event = TypeVar("_Event", contravariant=True)

tests/test_core/test_layout.py

@@ -11,7 +11,7 @@
 from idom import html
 from idom.config import IDOM_DEBUG_MODE
 from idom.core.component import component
-from idom.core.hooks import use_effect, use_state
+from idom.core.hooks import create_context, use_context, use_effect, use_state
 from idom.core.layout import Layout, LayoutEvent, LayoutUpdate
 from idom.testing import (
     HookCatcher,
@@ -20,7 +20,7 @@
     capture_idom_logs,
 )
 from idom.utils import Ref
-from tests.tooling.hooks import use_toggle
+from tests.tooling.hooks import use_force_render, use_toggle
 
 
 @pytest.fixture(autouse=True)
@@ -1123,83 +1123,6 @@ def Child():
         assert did_unmount.current
 
 
-class ComponentShouldRender:
-    def __init__(self, child, should_render):
-        self.child = child or html.div()
-        self.should_render = should_render
-        self.key = None
-        self.type = self.__class__
-
-    def render(self):
-        return html.div(self.child)
-
-
-async def test_component_should_render_always_true():
-    render_count = idom.Ref(0)
-    root_hook = HookCatcher()
-
-    @idom.component
-    @root_hook.capture
-    def Root():
-        return ComponentShouldRender(SomeComponent(), should_render=lambda new: True)
-
-    @idom.component
-    def SomeComponent():
-        render_count.current += 1
-        return html.div()
-
-    async with idom.Layout(Root()) as layout:
-        for _ in range(4):
-            await layout.render()
-            root_hook.latest.schedule_render()
-
-    assert render_count.current == 4
-
-
-async def test_component_should_render_always_false():
-    render_count = idom.Ref(0)
-    root_hook = HookCatcher()
-
-    @idom.component
-    @root_hook.capture
-    def Root():
-        return ComponentShouldRender(SomeComponent(), should_render=lambda new: False)
-
-    @idom.component
-    def SomeComponent():
-        render_count.current += 1
-        return html.div()
-
-    async with idom.Layout(Root()) as layout:
-        for _ in range(4):
-            await layout.render()
-            root_hook.latest.schedule_render()
-
-    assert render_count.current == 1
-
-
-async def test_component_error_in_should_render_is_handled_gracefully():
-    root_hook = HookCatcher()
-
-    @idom.component
-    @root_hook.capture
-    def Root():
-        def bad_should_render(new):
-            raise ValueError("The error message")
-
-        return ComponentShouldRender(html.div(), should_render=bad_should_render)
-
-    with assert_idom_did_log(
-        match_message=r".* component failed to check if .* should be rendered",
-        error_type=ValueError,
-        match_error="The error message",
-    ):
-        async with idom.Layout(Root()) as layout:
-            await layout.render()
-            root_hook.latest.schedule_render()
-            await layout.render()
-
-
 async def test_does_render_children_after_component():
     """Regression test for bug where layout was appending children to a stale ref
 
@@ -1221,7 +1144,6 @@ def Child():
 
     async with idom.Layout(Parent()) as layout:
         update = await layout.render()
-        print(update.new)
         assert update.new == {
             "tagName": "",
             "children": [
@@ -1238,3 +1160,34 @@ def Child():
                 }
             ],
         }
+
+
+async def test_remove_context_provider_consumer_without_changing_context_value():
+    Context = idom.create_context(None)
+    toggle_remove_child = None
+    schedule_removed_child_render = None
+
+    @component
+    def Parent():
+        nonlocal toggle_remove_child
+        remove_child, toggle_remove_child = use_toggle()
+        return Context(html.div() if remove_child else Child(), value=None)
+
+    @component
+    def Child():
+        nonlocal schedule_removed_child_render
+        schedule_removed_child_render = use_force_render()
+        return None
+
+    async with idom.Layout(Parent()) as layout:
+        await layout.render()
+
+        # If the context provider does not render its children then internally tracked
+        # state for the removed child component might not be cleaned up propperly
+        toggle_remove_child()
+        await layout.render()
+
+        # If this removed child component has state which has not been cleaned up
+        # correctly, scheduling a render for it might cause an error.
+        schedule_removed_child_render()
+        await layout.render()

tests/tooling/hooks.py

@@ -1,4 +1,8 @@
-from idom import use_state
+from idom.core.hooks import current_hook, use_state
+
+
+def use_force_render():
+    return current_hook().schedule_render
 
 
 def use_toggle(init=False):