diff --git a/docs/source/about/changelog.rst b/docs/source/about/changelog.rst index 617b09d57..10744b38a 100644 --- a/docs/source/about/changelog.rst +++ b/docs/source/about/changelog.rst @@ -23,7 +23,18 @@ more info, see the :ref:`Contributor Guide `. 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 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..e3151fcff 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 @@ -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() 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_hooks.py b/tests/test_core/test_hooks.py index ce45c433d..4a2faa07f 100644 --- a/tests/test_core/test_hooks.py +++ b/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() diff --git a/tests/test_core/test_layout.py b/tests/test_core/test_layout.py index 42c9d00c3..58648c3cb 100644 --- a/tests/test_core/test_layout.py +++ b/tests/test_core/test_layout.py @@ -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,43 @@ def Child(): } ], } + + +async def test_render_removed_context_consumer(): + 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() + + # If things were cleaned up propperly, the above scheduled render should not + # actually take place. Thus we expect the timeout to occur. + render_task = asyncio.create_task(layout.render()) + done, pending = await asyncio.wait([render_task], timeout=0.1) + assert not done and pending + render_task.cancel() 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):