From 0736668250804d2ef3decf29b1bce2f4d0da590a Mon Sep 17 00:00:00 2001 From: rmorshea Date: Sat, 7 Jan 2023 15:05:32 -0800 Subject: [PATCH] remove should render also adds a regression test in the case a component does not render its children --- src/idom/core/component.py | 3 - src/idom/core/hooks.py | 16 ----- src/idom/core/layout.py | 71 ++++++++------------ src/idom/core/types.py | 3 - tests/test_core/test_layout.py | 116 ++++++++++----------------------- tests/tooling/hooks.py | 6 +- 6 files changed, 68 insertions(+), 147 deletions(-) diff --git a/src/idom/core/component.py b/src/idom/core/component.py index 0bf62c9a7..ff4e4c655 100644 --- a/src/idom/core/component.py +++ b/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 diff --git a/src/idom/core/hooks.py b/src/idom/core/hooks.py index dd7d0fc4b..53bce9087 100644 --- a/src/idom/core/hooks.py +++ b/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})" diff --git a/src/idom/core/layout.py b/src/idom/core/layout.py index bbc1848a5..20166a2a3 100644 --- a/src/idom/core/layout.py +++ b/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: diff --git a/src/idom/core/types.py b/src/idom/core/types.py index 0ef45c263..b98a2aca2 100644 --- a/src/idom/core/types.py +++ b/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) diff --git a/tests/test_core/test_layout.py b/tests/test_core/test_layout.py index 42c9d00c3..47329b6e3 100644 --- a/tests/test_core/test_layout.py +++ b/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,37 @@ 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. This + # occured in the past when the context provider implemented a should_render() + # method that returned False (and thus did not render its children) when the + # context value did not change. + 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() diff --git a/tests/tooling/hooks.py b/tests/tooling/hooks.py index f6488af8a..36ed26419 100644 --- a/tests/tooling/hooks.py +++ b/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):