Conversation
ogrodnek
changed the title
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
+ Coverage 78.31% 78.38% +0.06%
==========================================
Files 99 100 +1
Lines 8362 8498 +136
Branches 620 632 +12
==========================================
+ Hits 6549 6661 +112
- Misses 1689 1708 +19
- Partials 124 129 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8f8372cf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logger.warning(f"Component {component_name} mount binding error: {err}") | ||
| raise ValueError(f"Component mount binding failed: {result.errors}") | ||
|
|
||
| await component.mount(**result.bound_args) |
There was a problem hiding this comment.
This switches component lifecycle invocation to component.mount(**result.bound_args) after name/type-based binding. Previously the manager called component.mount(socket, assigns) (and similar for update/handle_event), so parameter names didn’t matter. With the new kwargs-only call, any existing components that use nonstandard parameter names without socket/assigns/event/payload annotations (e.g., def mount(self, sock, props):) will now fail binding as “missing required parameter” because those names are not injectable. This is a backward‑compatibility regression for unannotated components unless they rename parameters or add type hints.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates LiveComponent lifecycle invocation to go through the dependency binder, enabling Depends() support in component mount, update, and handle_event.
Changes:
- Route LiveComponent lifecycle calls through new binding helpers (
call_component_mount/update/handle_event). - Extend injectable resolution to support socket injection by type and
assignsinjection by name. - Update tests and documentation to reflect LiveComponents DI usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/components/test_lifecycle.py | Renames lifecycle method parameter to socket in tests to align with binder-based injection. |
| tests/binding/test_injectables.py | Adds tests for socket type-based injection and assigns name-based injection. |
| pyview/components/manager.py | Switches component lifecycle/event calls to binder-driven helper functions. |
| pyview/components/binding.py | Adds new helper module that binds and calls component lifecycle methods with Depends() support. |
| pyview/binding/injectables.py | Adds socket type-based injectable resolution and assigns injectable support. |
| pyview/binding/context.py | Broadens BindContext.socket typing to allow component sockets. |
| docs/core-concepts/dependency-injection.md | Documents LiveComponents DI and adds an example showing Depends() usage in components. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _is_socket_annotation(self, annotation: Any) -> bool: | ||
| """Check if annotation is a socket type (ComponentSocket or LiveViewSocket).""" | ||
| from pyview.components.base import ComponentSocket # noqa: PLC0415 | ||
| from pyview.live_socket import ( # noqa: PLC0415 | ||
| ConnectedLiveViewSocket, | ||
| UnconnectedSocket, | ||
| ) | ||
|
|
||
| # Handle generic types like ComponentSocket[T] or LiveViewSocket[T] | ||
| origin = get_origin(annotation) | ||
| check_type = origin if origin is not None else annotation | ||
|
|
||
| socket_types = (ComponentSocket, ConnectedLiveViewSocket, UnconnectedSocket) |
There was a problem hiding this comment.
_is_socket_annotation() claims to handle generic types like LiveViewSocket[T], but LiveViewSocket is a TypeAlias to a Union[...] and this implementation only checks issubclass() on the origin type. As a result, a parameter annotated as LiveViewSocket[...] will not be recognized for type-based socket injection.
Suggestion: treat Union annotations as socket annotations when all union members are socket types (or are parameterized forms whose origin is a socket type). Add a regression test for LiveViewSocket[dict] / LiveViewSocket[Any] injection by type.
| ## LiveComponents | ||
|
|
||
| `Depends()` also works in LiveComponent lifecycle methods: | ||
|
|
||
| | Method | Async Deps | Session Available | | ||
| |--------|------------|-------------------| | ||
| | `mount` | Yes | No | | ||
| | `update` | Yes | No | | ||
| | `handle_event` | Yes | No | | ||
|
|
There was a problem hiding this comment.
This doc update introduces LiveComponents DI support, but the “Type-based injection” section below still only lists Session. With this PR, socket types can also be injected by type annotation (e.g., ComponentSocket, ConnectedLiveViewSocket, UnconnectedSocket).
Please update the “Available Injectables” docs to include the new type-based socket injections (and clarify whether LiveViewSocket aliases are supported).
| from pyview.components.base import LiveComponent, ComponentSocket | ||
|
|
||
| def get_formatter(): | ||
| return Formatter(locale="en-US") | ||
|
|
||
| class PriceDisplay(LiveComponent): |
There was a problem hiding this comment.
The new example uses Formatter(...) but doesn’t import or define Formatter, so the snippet won’t run as-written.
Suggestion: add an explicit import (or replace with a minimal stub/placeholder) so readers can copy/paste the example without hitting a NameError.
| async def call_component_mount( | ||
| component: LiveComponent, | ||
| socket: ComponentSocket, | ||
| assigns: dict[str, Any], | ||
| ) -> None: | ||
| """Bind and call component.mount() with Depends() support. | ||
|
|
||
| Args: | ||
| component: The LiveComponent instance | ||
| socket: The ComponentSocket instance | ||
| assigns: Props from parent template | ||
|
|
||
| Note: | ||
| Session is not available to components. Session-dependent data | ||
| should be passed from parent LiveView via assigns. | ||
| """ | ||
| ctx = BindContext( | ||
| params=Params({}), | ||
| payload=None, | ||
| url=None, | ||
| socket=socket, | ||
| event=None, | ||
| extra={"assigns": assigns}, | ||
| ) | ||
| binder = Binder() | ||
| result = await binder.abind(component.mount, ctx) | ||
|
|
There was a problem hiding this comment.
call_component_mount/update/handle_event introduce new behavior (Depends() + binder-driven argument binding) for component lifecycle methods, but there are no tests asserting that async Depends() resolution and payload/assigns binding work end-to-end for LiveComponents.
Add component-focused integration tests (similar to tests/binding/test_depends.py) that exercise: (1) async Depends() in component mount and update, (2) typed payload params in handle_event, and (3) caching behavior within a single lifecycle call.
ogrodnek
force-pushed
the
component_inject
branch
from
e1a6a6c to
ed64896
Compare
No description provided.