remove should_render (#870)

* remove should render

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

* changelog

* remove more unused code

* better test name

* fix style

Author: rmorshea

docs/source/about/changelog.rst

@@ -23,7 +23,18 @@ more info, see the :ref:`Contributor Guide <Creating a Changelog Entry>`.
 Unreleased
 ----------
 
-No changes.
+**Removed**
+
+- :pull:`870` - ``ComponentType.should_render()``. This method was implemented based on
+  reading the React/Preact source code. As it turns out though it seems like it's mostly
+  a vestige from the fact that both these libraries still support class-based
+  components. The ability for components to not render also caused several bugs.
+
+**Fixed**
+
+- :issue:`846` - Nested context does no update value if outer context should not render.
+- :issue:`847` - Detached model state on render of context consumer if unmounted and
+  context value does not change.
 
 
 v0.42.0

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
@@ -225,9 +216,6 @@ def _render_component(
                     *old_parent_children[index + 1 :],
                 ],
             }
-        finally:
-            # avoid double render
-            self._rendering_queue.remove_if_pending(life_cycle_state.id)
 
     def _render_model(
         self,
@@ -464,14 +452,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:
@@ -710,10 +690,6 @@ def put(self, value: _Type) -> None:
             self._loop.call_soon_threadsafe(self._queue.put_nowait, value)
         return None
 
-    def remove_if_pending(self, value: _Type) -> None:
-        if value in self._pending:
-            self._pending.remove(value)
-
     async def get(self) -> _Type:
         while True:
             value = await self._queue.get()

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_hooks.py

@@ -946,39 +946,6 @@ def test_context_repr():
     assert repr(sample_context()) == f"ContextProvider({sample_context})"
 
 
-async def test_use_context_only_renders_for_value_change():
-    Context = idom.create_context(None)
-
-    provider_hook = HookCatcher()
-    render_count = idom.Ref(0)
-    set_state = idom.Ref()
-
-    @idom.component
-    @provider_hook.capture
-    def ComponentProvidesContext():
-        state, set_state.current = idom.use_state(0)
-        return Context(ComponentInContext(), value=state)
-
-    @idom.component
-    def ComponentInContext():
-        render_count.current += 1
-        return html.div()
-
-    async with idom.Layout(ComponentProvidesContext()) as layout:
-        await layout.render()
-        assert render_count.current == 1
-
-        set_state.current(1)
-
-        await layout.render()
-        assert render_count.current == 2
-
-        provider_hook.latest.schedule_render()
-
-        await layout.render()
-        assert render_count.current == 2
-
-
 async def test_use_context_updates_components_even_if_memoized():
     Context = idom.create_context(None)
 
@@ -1019,114 +986,26 @@ def MemoizedComponentUsesContext():
         assert value.current == 2
 
 
-async def test_nested_contexts_do_not_conflict():
+async def test_context_values_are_scoped():
     Context = idom.create_context(None)
 
-    outer_value = idom.Ref(None)
-    inner_value = idom.Ref(None)
-    outer_render_count = idom.Ref(0)
-    inner_render_count = idom.Ref(0)
-    set_outer_value = idom.Ref()
-    set_root_value = idom.Ref()
-
-    @idom.component
-    def Root():
-        outer_value, set_root_value.current = idom.use_state(-1)
-        return Context(Outer(), value=outer_value)
-
-    @idom.component
-    def Outer():
-        inner_value, set_outer_value.current = idom.use_state(1)
-        outer_value.current = idom.use_context(Context)
-        outer_render_count.current += 1
-        return Context(Inner(), value=inner_value)
-
-    @idom.component
-    def Inner():
-        inner_value.current = idom.use_context(Context)
-        inner_render_count.current += 1
-        return html.div()
-
-    async with idom.Layout(Root()) as layout:
-        await layout.render()
-        assert outer_render_count.current == 1
-        assert inner_render_count.current == 1
-        assert outer_value.current == -1
-        assert inner_value.current == 1
-
-        set_root_value.current(-2)
-
-        await layout.render()
-        assert outer_render_count.current == 2
-        assert inner_render_count.current == 1
-        assert outer_value.current == -2
-        assert inner_value.current == 1
-
-        set_outer_value.current(2)
-
-        await layout.render()
-        assert outer_render_count.current == 3
-        assert inner_render_count.current == 2
-        assert outer_value.current == -2
-        assert inner_value.current == 2
-
-
-async def test_neighboring_contexts_do_not_conflict():
-    LeftContext = idom.create_context(None)
-    RightContext = idom.create_context(None)
-
-    set_left = idom.Ref()
-    set_right = idom.Ref()
-    left_used_value = idom.Ref()
-    right_used_value = idom.Ref()
-    left_render_count = idom.Ref(0)
-    right_render_count = idom.Ref(0)
-
     @idom.component
-    def Root():
-        left_value, set_left.current = idom.use_state(1)
-        right_value, set_right.current = idom.use_state(1)
-        return idom.html.div(
-            LeftContext(Left(), value=left_value),
-            RightContext(Right(), value=right_value),
+    def Parent():
+        return html._(
+            Context(Context(Child1(), value=1), value="something-else"),
+            Context(Child2(), value=2),
         )
 
     @idom.component
-    def Left():
-        left_render_count.current += 1
-        left_used_value.current = idom.use_context(LeftContext)
-        return idom.html.div()
+    def Child1():
+        assert idom.use_context(Context) == 1
 
     @idom.component
-    def Right():
-        right_render_count.current += 1
-        right_used_value.current = idom.use_context(RightContext)
-        return idom.html.div()
+    def Child2():
+        assert idom.use_context(Context) == 2
 
-    async with idom.Layout(Root()) as layout:
+    async with Layout(Parent()) as layout:
         await layout.render()
-        assert left_render_count.current == 1
-        assert right_render_count.current == 1
-        assert left_used_value.current == 1
-        assert right_used_value.current == 1
-
-        for i in range(2, 5):
-            set_left.current(i)
-
-            await layout.render()
-            assert left_render_count.current == i
-            assert right_render_count.current == 1
-            assert left_used_value.current == i
-            assert right_used_value.current == 1
-
-        for j in range(2, 5):
-            set_right.current(j)
-
-            await layout.render()
-            assert left_render_count.current == i
-            assert right_render_count.current == j
-            assert left_used_value.current == i
-            assert right_used_value.current == j
 
 
 async def test_error_in_effect_cleanup_is_gracefully_handled():
@@ -1357,30 +1236,6 @@ def incr_effect_count():
         assert effect_count.current == 1
 
 
-@pytest.mark.parametrize("get_value", STRICT_EQUALITY_VALUE_CONSTRUCTORS)
-async def test_use_context_compares_with_strict_equality(get_value):
-    hook = HookCatcher()
-    context = idom.create_context(None)
-    inner_render_count = idom.Ref(0)
-
-    @idom.component
-    @hook.capture
-    def OuterComponent():
-        return context(InnerComponent(), value=get_value())
-
-    @idom.component
-    def InnerComponent():
-        idom.use_context(context)
-        inner_render_count.current += 1
-
-    async with idom.Layout(OuterComponent()) as layout:
-        await layout.render()
-        assert inner_render_count.current == 1
-        hook.latest.schedule_render()
-        await layout.render()
-        assert inner_render_count.current == 1
-
-
 async def test_use_state_named_tuple():
     state = idom.Ref()