From 7acf68e2eaa4eb6812b9ac9a792daba10b63ea84 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Sat, 12 Sep 2026 18:05:54 -0400 Subject: [PATCH 1/3] fix(py): accept the `dict` every app returns; check tests and examples Widening pyright past `pkg-py/src/shinyreact` surfaced two annotations that were unusable from user code -- which is the argument for widening it. `pkg-py/tests` and `examples` are the only place the *caller's* view of the API is exercised, so nothing else could have caught either. - `reactive_output` and `send_message(data=)` were typed `Jsonifiable`, whose containers are `dict` and `list`. Both are invariant in their element types, so a render function returning the natural `dict[str, int]` was not assignable to it: the single most ordinary thing a `reactive_output` does was a type error at every call site, five example apps included. They now take `JsonValue`, the same union spelled with covariant `Mapping` / `Sequence`; `Jsonifiable` stays the type on the way out to Shiny. - `page_react_html()` declared `HTMLTextDocument`, but `App(ui=)` accepts only its `PageHtmlDocument` subclass -- so the documented plain-`shiny.App` path failed to type-check. `pkg-py/tests/test_typing.py` pins both. It asserts nothing at runtime; pyright failing is the test. Reverting either fix produces 13 errors. Three tooling gaps closed alongside: - `py-check-format` ran `ruff check` only, never `ruff format --check`, so the formatter's output was unenforced -- and one file had already drifted. - Both ruff targets scoped to `pkg-py`, leaving `examples/` unlinted and unformatted despite being shipped, read as documentation, and run by the default pytest invocation. Both now cover `$(PATHS_PY)`. - The `tests-e2e` group asked for a `shiny[playwright]` extra py-shiny does not have, which uv reports as a warning rather than an error on every sync -- exactly how a real extras problem would hide. The remaining pre-existing errors in tests and examples are fixed in place: casts where a test deliberately passes `None` for an argument the handler ignores, narrowing helpers in `test_dep.py` for `HTMLDependency`'s wider-than-actual attribute types, and `.loc[:, ...]` in `04-shadcn` because pandas types `df[[...]]` as `Series | Unknown`. --- FEATURES.md | 19 ++++- Makefile | 12 ++- examples/04-shadcn/app.py | 4 +- pkg-py/src/shinyreact/_json.py | 28 ++++++ pkg-py/src/shinyreact/_page.py | 12 ++- pkg-py/src/shinyreact/_reactive_output.py | 13 ++- pkg-py/src/shinyreact/_send_message.py | 8 +- pkg-py/tests/playwright/test_output_error.py | 4 +- pkg-py/tests/test_bookmark_restore.py | 5 +- pkg-py/tests/test_dep.py | 54 ++++++++---- pkg-py/tests/test_dep_discovery.py | 13 ++- pkg-py/tests/test_input_handler.py | 36 +++++--- pkg-py/tests/test_set_react_page.py | 6 +- pkg-py/tests/test_typing.py | 90 ++++++++++++++++++++ pyproject.toml | 16 +++- 15 files changed, 263 insertions(+), 57 deletions(-) create mode 100644 pkg-py/src/shinyreact/_json.py create mode 100644 pkg-py/tests/test_typing.py diff --git a/FEATURES.md b/FEATURES.md index b703082a..230e33ec 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -192,7 +192,7 @@ R has no e2e suite, so no `(e2e)` leaf covers R (issue #194). - it accepts any JSON-serializable value - the client reads it with `useShinyOutputValue(id)` - call shape — **deliberate divergence** - - `[py]` a `Renderer[Jsonifiable]` subclass, used as a decorator, assigned + - `[py]` a `Renderer[JsonValue]` subclass, used as a decorator, assigned to `output[id]` - `[r]` a function, called as `output$id <- reactive_output(expr, ...)` - reason: each language's renderer idiom — a Python `Renderer` subclass has @@ -201,6 +201,17 @@ R has no e2e suite, so no `(e2e)` leaf covers R (issue #194). never a #184 parity question - `[py]` accepted types are whatever `Jsonifiable` admits: `dict`, `list`, `tuple`, `str`, `int`, `float`, `bool`, `None` + - spelled as `shinyreact._json.JsonValue`, whose containers are `Mapping` / + `Sequence` rather than `Jsonifiable`'s `dict` / `list` + - `dict` and `list` are **invariant** in their element types, so a render + function returning `dict[str, int]` is not assignable to `Jsonifiable` + — a type error at the most ordinary call site there is + - `Mapping` / `Sequence` are covariant, so `dict[str, int]`, + `dict[str, list[float]]` and `list[dict[str, str]]` all type-check with + no annotation at the call site + - same alias on `send_message(data=)`, for the same reason + - pinned by `pkg-py/tests/test_typing.py`, which asserts nothing at + runtime — pyright failing is the test - `[py]` a `str` return is sent as a JSON string, not a text node - `[py]` `auto_output_ui()` returns `None`, inherited from `Renderer` — there is no placeholder element to emit @@ -859,6 +870,12 @@ the shinyreact bundle dependency and the `#shinyreact-config` tag — except - it works as `shiny.App(ui=...)`, but only `ReactApp` mounts the document's directory — under plain `shiny.App` the sibling `ui.js` is not served + - the declared return type is py-shiny's `PageHtmlDocument`, which is + what `App(ui=)` accepts; its `HTMLTextDocument` base is **not** + accepted, so declaring the base would make that documented path a type + error + - `PageHtmlDocument` is imported from `shiny.ui._page`: py-shiny exports + `page_html()` but not its class - `shiny.ui.page_html()` arrives with py-shiny#2475, consumed as a git dependency on py-shiny `main` until it releases - `[r]` used directly as `shinyApp(ui = page_react_html())`, implemented by diff --git a/Makefile b/Makefile index 79f5f609..3a2ae01f 100644 --- a/Makefile +++ b/Makefile @@ -203,16 +203,22 @@ py-check-types: ## [py] Run python type checks @echo "📝 Checking types with pyright" uv run pyright +# Both targets cover `examples/` as well as `pkg-py/`: the example apps and +# their tests are shipped, read as documentation, and run by the default pytest +# invocation, so they are held to the same bar as the package. +PATHS_PY = pkg-py examples + .PHONY: py-check-format py-check-format: ## [py] Check python formatting @echo "" @echo "📐 Checking format with ruff" - uv run ruff check pkg-py --config pyproject.toml + uv run ruff check $(PATHS_PY) --config pyproject.toml + uv run ruff format --check $(PATHS_PY) --config pyproject.toml .PHONY: py-format py-format: ## [py] Format python code - uv run ruff check --fix pkg-py --config pyproject.toml - uv run ruff format pkg-py --config pyproject.toml + uv run ruff check --fix $(PATHS_PY) --config pyproject.toml + uv run ruff format $(PATHS_PY) --config pyproject.toml .PHONY: py-install-e2e py-install-e2e: ## [py] Install Playwright browsers for e2e tests diff --git a/examples/04-shadcn/app.py b/examples/04-shadcn/app.py index 660e46e3..ac313343 100644 --- a/examples/04-shadcn/app.py +++ b/examples/04-shadcn/app.py @@ -35,7 +35,9 @@ @reactive_output def scatter_data(): - return sample_data[["age", "score"]].to_dict(orient="list") + # `.loc[:, [...]]`, not `[[...]]`: pandas types the latter as + # `Series | Unknown`, and `Series.to_dict()` takes no `orient=`. + return sample_data.loc[:, ["age", "score"]].to_dict(orient="list") @reactive_output diff --git a/pkg-py/src/shinyreact/_json.py b/pkg-py/src/shinyreact/_json.py new file mode 100644 index 00000000..df89a2ab --- /dev/null +++ b/pkg-py/src/shinyreact/_json.py @@ -0,0 +1,28 @@ +from __future__ import annotations + +from typing import Mapping, Sequence, Union + +JsonValue = Union[ + str, + int, + float, + bool, + None, + Sequence["JsonValue"], + Mapping[str, "JsonValue"], +] +"""Any JSON-serializable value, as accepted from user code. + +Shiny's ``Jsonifiable`` spells its containers as ``list`` and ``dict``, both of +which are **invariant** in their element types. That makes the natural thing a +caller writes -- a function returning ``dict[str, int]``, or a +``dict[str, str]`` variable -- unassignable to it, so the most common +``reactive_output`` and ``send_message`` payloads fail to type-check for no +runtime reason. + +``Sequence`` and ``Mapping`` are covariant in their element types, so this +alias accepts exactly the values ``Jsonifiable`` describes without forcing +every call site to annotate its own return type as ``Jsonifiable``. Use it on +the way *in*, from user code; ``Jsonifiable`` stays the type on the way *out*, +to Shiny. +""" diff --git a/pkg-py/src/shinyreact/_page.py b/pkg-py/src/shinyreact/_page.py index b00b612b..0fb03ef6 100644 --- a/pkg-py/src/shinyreact/_page.py +++ b/pkg-py/src/shinyreact/_page.py @@ -5,12 +5,18 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Callable, cast -from htmltools import HTML, HTMLDependency, HTMLTextDocument, Tag, TagChild, TagList +from htmltools import HTML, HTMLDependency, Tag, TagChild, TagList from shiny.express.ui import page_opts from shiny.render.renderer import Renderer from shiny.session import get_current_session from shiny.ui import page_html +# The class `page_html()` returns. `App(ui=)` accepts it but not its +# `HTMLTextDocument` base, so declaring the base as our return type would make +# the documented `App(page_react_html(...), server)` fail to type-check. +# py-shiny exports the function but not the class -- hence the private import. +from shiny.ui._page import PageHtmlDocument + from ._app import SRC_DIR_ATTR from ._bookmark import _config_script_tag from ._dep import ShinyreactJs, _dep, _dep_page, _file_mtime_int, _serves_bundle @@ -437,7 +443,7 @@ def page_react_html( *, extra_deps: list[HTMLDependency] | None = None, shinyreact_js: ShinyreactJs = "server", -) -> HTMLTextDocument: +) -> PageHtmlDocument: """Serve a React ``index.html`` document (the ui.tsx pattern, Core API). Reads a complete HTML document — the kind a Vite build emits — and injects @@ -507,7 +513,7 @@ def page_react_html( *(extra_deps or []), ], ) - # Tagged, not subclassed: py-shiny exports page_html() but not its class. + # Tagged, not subclassed: py-shiny does not export the class publicly. # ReactApp reads this to mount the document's directory at "/". setattr(doc, SRC_DIR_ATTR, index_path.parent) return doc diff --git a/pkg-py/src/shinyreact/_reactive_output.py b/pkg-py/src/shinyreact/_reactive_output.py index 5d041d5c..9c0e3893 100644 --- a/pkg-py/src/shinyreact/_reactive_output.py +++ b/pkg-py/src/shinyreact/_reactive_output.py @@ -1,10 +1,14 @@ from __future__ import annotations +from typing import cast + from shiny.render.renderer import Renderer from shiny.types import Jsonifiable +from ._json import JsonValue + -class reactive_output(Renderer["Jsonifiable"]): +class reactive_output(Renderer["JsonValue"]): """Publish a reactive JSON value to the client (the ``ui.tsx`` pattern). Assign to ``output[id]`` where a React client reads the value with @@ -15,5 +19,8 @@ class reactive_output(Renderer["Jsonifiable"]): ``float``, ``bool``, ``None``), passed through unchanged. """ - async def transform(self, value: Jsonifiable) -> Jsonifiable: - return value + async def transform(self, value: JsonValue) -> Jsonifiable: + # `JsonValue` and `Jsonifiable` describe the same runtime values; only + # their container variance differs, so this is a re-labeling, not a + # conversion. + return cast(Jsonifiable, value) diff --git a/pkg-py/src/shinyreact/_send_message.py b/pkg-py/src/shinyreact/_send_message.py index 1972beeb..6b4252dd 100644 --- a/pkg-py/src/shinyreact/_send_message.py +++ b/pkg-py/src/shinyreact/_send_message.py @@ -1,9 +1,11 @@ from __future__ import annotations -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast from shiny.module import resolve_id +from ._json import JsonValue + if TYPE_CHECKING: from shiny.session import Session from shiny.types import Jsonifiable @@ -12,7 +14,7 @@ async def send_message( session: Session, id: str, - data: Jsonifiable, + data: JsonValue, ) -> None: """Send a custom message from server to client React components. @@ -36,5 +38,5 @@ async def notify(): """ namespaced_id = resolve_id(id) await session.send_custom_message( - "shinyReactMessage", {"id": namespaced_id, "data": data} + "shinyReactMessage", {"id": namespaced_id, "data": cast("Jsonifiable", data)} ) diff --git a/pkg-py/tests/playwright/test_output_error.py b/pkg-py/tests/playwright/test_output_error.py index 9d2949cf..e9e48ae1 100644 --- a/pkg-py/tests/playwright/test_output_error.py +++ b/pkg-py/tests/playwright/test_output_error.py @@ -74,8 +74,6 @@ def test_empty_message_error_is_not_logged_to_the_console( # A real error does log, so the assertion above has teeth. page.locator("[data-test=input]").fill("0") - expect(page.locator("[data-test=error]")).to_have_text( - "invalid number of 'breaks'" - ) + expect(page.locator("[data-test=error]")).to_have_text("invalid number of 'breaks'") expect(page.locator("[data-test=status]")).to_have_text("error") assert [m for m in logged if "Error for answer:" in m] != [] diff --git a/pkg-py/tests/test_bookmark_restore.py b/pkg-py/tests/test_bookmark_restore.py index bb918763..70b40df4 100644 --- a/pkg-py/tests/test_bookmark_restore.py +++ b/pkg-py/tests/test_bookmark_restore.py @@ -1,6 +1,7 @@ import json import re from pathlib import Path +from typing import Mapping import pytest from htmltools import HTMLDependency, TagList @@ -70,7 +71,9 @@ def _extract_restore_payload(head_html: str) -> object: return config["restore"] -def _config_html(values: dict[str, object]) -> str: +# `Mapping`, not `dict`: `dict` is invariant in its value type, so a caller's +# `dict[str, str]` literal would not be assignable. +def _config_html(values: Mapping[str, object]) -> str: ctx = RestoreContext() ctx.input = RestoreInputSet(dict(values)) with restore_context_cm(ctx): diff --git a/pkg-py/tests/test_dep.py b/pkg-py/tests/test_dep.py index 4aebd420..874bef11 100644 --- a/pkg-py/tests/test_dep.py +++ b/pkg-py/tests/test_dep.py @@ -1,13 +1,38 @@ import warnings from pathlib import Path +from typing import cast import pytest import shinyreact._dep as _dep_mod from htmltools import HTMLDependency +from htmltools._core import HTMLDependencySource, ScriptItem, StylesheetItem from shinyreact._dep import _SHINYREACT_JS_PATH, _dep from shinyreact._page import page_react_dep +# `HTMLDependency` normalizes `script=` / `stylesheet=` to a list and `source=` +# to one of two TypedDicts, but the attributes stay declared as the wider +# argument types. These narrow once here so the assertions below read as +# assertions rather than as type gymnastics. +def dep_source(dep: HTMLDependency) -> HTMLDependencySource: + source = dep.source + assert source is not None + assert "subdir" in source, "a local dep, not an href one" + return cast(HTMLDependencySource, source) + + +def first_script(dep: HTMLDependency) -> ScriptItem: + script = dep.script + assert script is not None + return script[0] if isinstance(script, list) else script + + +def first_stylesheet(dep: HTMLDependency) -> StylesheetItem: + stylesheet = dep.stylesheet + assert stylesheet is not None + return stylesheet[0] if isinstance(stylesheet, list) else stylesheet + + def test_dep_version_tracks_bundle_mtime(): """The shinyreact HTMLDependency version reflects the bundle's mtime. @@ -21,11 +46,7 @@ def test_dep_version_tracks_bundle_mtime(): def test_dep_script_has_defer(): - script = _dep().script - assert script is not None - if isinstance(script, list): - script = script[0] - assert script.get("defer") == "" + assert first_script(_dep()).get("defer") == "" # --- page_react_dep tests --- @@ -57,7 +78,7 @@ def test_page_react_dep_returns_htmldependency(tmp_path): dep = _run_page_react_dep(tmp_path) assert isinstance(dep, HTMLDependency) - assert dep.source["subdir"] == str(tmp_path) + assert dep_source(dep)["subdir"] == str(tmp_path) assert dep.name == tmp_path.name @@ -133,7 +154,7 @@ def test_page_react_dep_attaches_script_when_js_present(tmp_path): (tmp_path / "ui.js").write_text("// app") dep = page_react_dep(src_dir=tmp_path) - script = dep.script if isinstance(dep.script, dict) else dep.script[0] + script = first_script(dep) assert script["src"] == "ui.js" assert script.get("type") == "module" @@ -143,10 +164,8 @@ def test_page_react_dep_custom_filenames(tmp_path): (tmp_path / "app.css").write_text("/* styles */") dep = _run_page_react_dep(tmp_path, js_file="app.js", css_file="app.css") - script = dep.script if isinstance(dep.script, dict) else dep.script[0] - stylesheet = ( - dep.stylesheet if isinstance(dep.stylesheet, dict) else dep.stylesheet[0] - ) + script = first_script(dep) + stylesheet = first_stylesheet(dep) assert script["src"] == "app.js" assert stylesheet["href"] == "app.css" @@ -156,7 +175,7 @@ def test_page_react_dep_script_type_module(tmp_path): (tmp_path / "ui.css").write_text("/* styles */") dep = _run_page_react_dep(tmp_path) - script = dep.script if isinstance(dep.script, dict) else dep.script[0] + script = first_script(dep) assert script.get("type") == "module" @@ -169,8 +188,7 @@ def test_page_react_dep_explicit_src_dir_and_name(tmp_path): (tmp_path / "ui.js").write_text("// app") dep = page_react_dep(src_dir=tmp_path, name="my-app") - assert dep.source is not None - assert dep.source["subdir"] == str(tmp_path) + assert dep_source(dep)["subdir"] == str(tmp_path) assert dep.name == "my-app" assert str(dep.version) == str(int((tmp_path / "ui.js").stat().st_mtime)) @@ -188,9 +206,7 @@ def test_page_react_dep_attaches_stylesheet_when_css_present(tmp_path): (tmp_path / "ui.js").write_text("// app") (tmp_path / "ui.css").write_text("/* styles */") - stylesheet = page_react_dep(src_dir=tmp_path).stylesheet - assert stylesheet is not None - entry = stylesheet if isinstance(stylesheet, dict) else stylesheet[0] + entry = first_stylesheet(page_react_dep(src_dir=tmp_path)) assert entry["href"] == "ui.css" @@ -198,7 +214,9 @@ def test_dep_stylesheet_attached_unconditionally() -> None: # No existence check on the CSS, unlike page_react_dep()'s. Mirrors R's # "shinyreact_dep() attaches the stylesheet unconditionally". dep = _dep() - assert [s["href"] for s in dep.stylesheet] == ["shinyreact.css"] + assert [s["href"] for s in cast("list[StylesheetItem]", dep.stylesheet)] == [ + "shinyreact.css" + ] def test_dep_version_falls_back_when_bundle_missing( diff --git a/pkg-py/tests/test_dep_discovery.py b/pkg-py/tests/test_dep_discovery.py index b064e65a..0eceb90a 100644 --- a/pkg-py/tests/test_dep_discovery.py +++ b/pkg-py/tests/test_dep_discovery.py @@ -9,11 +9,12 @@ from __future__ import annotations import asyncio -from typing import Any, Callable +from typing import Any, Callable, cast from htmltools import HTMLDependency, TagChild, TagList, div from shiny.express._stub_session import ExpressStubSession from shiny.input_handler import input_handlers +from shiny.module import ResolvedId from shiny.render.renderer import Renderer from shiny.session import session_context from shinyreact._dep_discovery import install_dep_discovery @@ -119,6 +120,10 @@ def test_install_is_idempotent_per_session() -> None: assert len(session.messages) == 1 +# The input id a handler is given; none of these handlers look at it. +ANY_NAME = cast(ResolvedId, "x") + + def test_install_no_ops_without_a_real_session() -> None: assert install_dep_discovery(None) is False assert install_dep_discovery(object()) is False # type: ignore[arg-type] @@ -126,12 +131,12 @@ def test_install_no_ops_without_a_real_session() -> None: def test_init_handler_installs_discovery() -> None: session = _FakeSession() - assert input_handlers["shinyreact.init"](1, "x", session) == 1 + assert input_handlers["shinyreact.init"](1, ANY_NAME, session) == 1 assert session.flush_callbacks != [] def test_value_handlers_do_not_install_discovery() -> None: session = _FakeSession() - input_handlers["shinyreact.default"]([], "x", session) - input_handlers["shinyreact.asis"](1, "x", session) + input_handlers["shinyreact.default"]([], ANY_NAME, session) + input_handlers["shinyreact.asis"](1, ANY_NAME, session) assert session.flush_callbacks == [] diff --git a/pkg-py/tests/test_input_handler.py b/pkg-py/tests/test_input_handler.py index 5f38485c..0ac2ac33 100644 --- a/pkg-py/tests/test_input_handler.py +++ b/pkg-py/tests/test_input_handler.py @@ -1,8 +1,18 @@ import importlib +from typing import cast import shinyreact # noqa: F401 (import registers the handlers) import shinyreact._input_handler from shiny.input_handler import input_handlers +from shiny.module import ResolvedId +from shiny.session import Session + +# Shiny hands every input handler `(value, name, session)`. Both shinyreact +# handlers ignore the last two, which is part of what these tests assert, so +# `None` stands in for them -- the casts say that is the point, not a missing +# fixture. +NO_NAME = cast(ResolvedId, None) +NO_SESSION = cast(Session, None) def test_all_handlers_are_registered(): @@ -17,21 +27,21 @@ def test_all_handlers_are_registered(): def test_default_handler_returns_value_unchanged(): handler = input_handlers["shinyreact.default"] records = [{"name": "a", "size": 1}, {"name": "b", "size": 2}] - assert handler(records, None, None) == records - assert handler([0, 100], None, None) == [0, 100] - assert handler(5, None, None) == 5 - assert handler([], None, None) == [] - assert handler(None, None, None) is None + assert handler(records, NO_NAME, NO_SESSION) == records + assert handler([0, 100], NO_NAME, NO_SESSION) == [0, 100] + assert handler(5, NO_NAME, NO_SESSION) == 5 + assert handler([], NO_NAME, NO_SESSION) == [] + assert handler(None, NO_NAME, NO_SESSION) is None def test_asis_handler_returns_value_unchanged(): handler = input_handlers["shinyreact.asis"] records = [{"name": "a"}, {"name": "b"}] - assert handler(records, None, None) == records - assert handler([0, 100], None, None) == [0, 100] - assert handler(5, None, None) == 5 - assert handler([], None, None) == [] - assert handler(None, None, None) is None + assert handler(records, NO_NAME, NO_SESSION) == records + assert handler([0, 100], NO_NAME, NO_SESSION) == [0, 100] + assert handler(5, NO_NAME, NO_SESSION) == 5 + assert handler([], NO_NAME, NO_SESSION) == [] + assert handler(None, NO_NAME, NO_SESSION) is None def test_default_handler_preserves_nested_structures(): @@ -42,10 +52,10 @@ def test_default_handler_preserves_nested_structures(): servers. This test is the Python half of that contract. """ handler = input_handlers["shinyreact.default"] - assert handler([[1, 2], [3, 4]], None, None) == [[1, 2], [3, 4]] - assert handler([{"a": 1}, 5], None, None) == [{"a": 1}, 5] + assert handler([[1, 2], [3, 4]], NO_NAME, NO_SESSION) == [[1, 2], [3, 4]] + assert handler([{"a": 1}, 5], NO_NAME, NO_SESSION) == [{"a": 1}, 5] # An empty array stays an empty array, not None. - assert handler([], None, None) == [] + assert handler([], NO_NAME, NO_SESSION) == [] def test_reregistration_is_idempotent(): diff --git a/pkg-py/tests/test_set_react_page.py b/pkg-py/tests/test_set_react_page.py index ce8f46fc..be19ae3b 100644 --- a/pkg-py/tests/test_set_react_page.py +++ b/pkg-py/tests/test_set_react_page.py @@ -3,17 +3,17 @@ from unittest.mock import patch import pytest -from htmltools import Tag +from htmltools import RenderedHTML, Tag from shiny import render from shinyreact import reactive_output, set_react_page from shinyreact._page import _build_react_page_fn, _build_react_page_fn_discovered -def _render(page_fn: Callable[..., Tag], *args: Any) -> dict[str, Any]: +def _render(page_fn: Callable[..., Tag], *args: Any) -> RenderedHTML: return page_fn(*args).tagify().render() -def _render_with(page_fn: Callable[..., Tag], **kwargs: Any) -> dict[str, Any]: +def _render_with(page_fn: Callable[..., Tag], **kwargs: Any) -> RenderedHTML: """Render a page_fn the way page_auto() calls it: options as kwargs.""" return page_fn(**kwargs).tagify().render() diff --git a/pkg-py/tests/test_typing.py b/pkg-py/tests/test_typing.py new file mode 100644 index 00000000..ff57f010 --- /dev/null +++ b/pkg-py/tests/test_typing.py @@ -0,0 +1,90 @@ +"""The public API as a *caller* sees it, checked by pyright rather than pytest. + +These functions assert nothing at runtime -- they barely run. Their job is to +fail `make py-check-types` if a signature stops accepting the values user code +actually produces, which no runtime test can catch: an over-narrow annotation +still passes every assertion in `test_reactive_output.py`. + +Each return type below is written out deliberately. An unannotated +`def f(): return {"a": 1}` would be inferred, and inference is exactly what +hides the bug: `dict[str, int]` is not assignable to `dict[str, Jsonifiable]` +because `dict` is invariant in its value type, so the most ordinary thing a +`reactive_output` does used to be a type error at every call site (every +example app included). `Mapping` / `Sequence` are covariant, hence +`shinyreact._json.JsonValue`. +""" + +from __future__ import annotations + +from pathlib import Path + +from shiny import App, Inputs, Outputs, Session +from shinyreact import ReactApp, page_react_html, reactive_output, send_message + + +@reactive_output +def dict_of_int() -> dict[str, int]: + return {"a": 1} + + +@reactive_output +def dict_of_list() -> dict[str, list[float]]: + return {"breaks": [1.0, 2.0]} + + +@reactive_output +def nested_dict() -> dict[str, dict[str, list[str]]]: + return {"cols": {"A": ["Apple"]}} + + +@reactive_output +def list_of_dict() -> list[dict[str, str]]: + return [{"name": "a"}] + + +@reactive_output +def plain_str() -> str: + return "hello" + + +@reactive_output +def plain_none() -> None: + return None + + +async def send_typed_values(session: Session, counts: dict[str, int]) -> None: + """`send_message()` takes the same JSON values `reactive_output` returns. + + A dict *literal* would be inferred against the parameter type and pass + either way; a `dict[str, int]` variable is what catches the invariance. + """ + await send_message(session, "counts", counts) + await send_message(session, "names", ["a", "b"]) + + +def html_document_is_accepted_as_app_ui(index: Path) -> App: + """`page_react_html()` returns what `App(ui=)` accepts. + + `App(ui=)` takes py-shiny's `PageHtmlDocument`, not its wider + `HTMLTextDocument` base, so declaring the base here would make the + documented plain-`shiny.App` path fail to type-check. + """ + + def server(i: Inputs, o: Outputs, s: Session) -> None: ... + + return App(page_react_html(index), server) + + +def react_app_is_a_shiny_app(index: Path) -> App: + def server(i: Inputs, o: Outputs, s: Session) -> None: ... + + return ReactApp(server, ui=page_react_html(index)) + + +def test_module_type_checks() -> None: + """A pytest anchor, so the file is not mistaken for dead code. + + The real assertion is that pyright reports no error for this module; see + `[tool.pyright] include` in `pyproject.toml`. + """ + assert dict_of_int.auto_output_ui() is None diff --git a/pyproject.toml b/pyproject.toml index dc014fc5..95dc5892 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -51,8 +51,12 @@ docs = [ "griffe<2", ] tests-e2e = [ + # `pytest-playwright` pulls in `playwright` itself. py-shiny has no + # `playwright` extra (theme, otel, test, dev, doc, add-test), and asking for + # one uv cannot find is a warning on every `uv sync`, not an error -- which + # is exactly how a real extras problem would hide. "pytest-playwright>=0.5.0", - "shiny[playwright] @ git+https://github.com/posit-dev/py-shiny.git@main", + "shiny @ git+https://github.com/posit-dev/py-shiny.git@main", "shinywidgets>=0.8.0", "plotly>=6.7.0", "pandas>=2.1.0", @@ -77,8 +81,18 @@ testpaths = ["pkg-py/tests", "examples"] addopts = "--ignore=pkg-py/tests/playwright" [tool.pyright] +# Tests and examples are checked too, not just the package. They are the only +# place the *caller's* view of the API is exercised, so a signature that is +# unusable from user code -- a `reactive_output` that rejects the `dict` every +# app returns -- shows up here and nowhere else. include = [ "pkg-py/src/shinyreact", + "pkg-py/tests", + "examples", +] +exclude = [ + "examples/shiny-react-upstream", + "**/node_modules", ] pythonVersion = "3.10" typeCheckingMode = "basic" From 21d5f2775927aaa35600a92f4b97310e6406f253 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Sat, 12 Sep 2026 18:35:37 -0400 Subject: [PATCH 2/3] fix(py): type-check tests and examples; enforce the formatter Widening pyright past `pkg-py/src/shinyreact` surfaced one annotation of ours that was unusable from user code, and one of py-shiny's. `page_react_html()` declared `HTMLTextDocument`, but `App(ui=)` accepts only its `PageHtmlDocument` subclass, so the documented plain-`shiny.App` path failed to type-check. Fixed here, and pinned by `pkg-py/tests/test_typing.py`, which asserts nothing at runtime -- pyright failing is the test. The other is upstream: `Jsonifiable`'s `dict` / `list` arms are invariant in their element types, so a render function returning `dict[str, int]` is not assignable to it -- the single most ordinary thing a `reactive_output` does, a type error in five example apps. That is posit-dev/py-shiny#2497; suppressed per call site with a `# pyright: ignore[reportArgumentType]` naming the issue rather than worked around locally, so the fix lands once, upstream. Grep `2497` to find every site when it does. Three tooling gaps closed alongside: - `py-check-format` ran `ruff check` only, never `ruff format --check`, so the formatter's output was unenforced -- and one file had already drifted. - Both ruff targets scoped to `pkg-py`, leaving `examples/` unlinted and unformatted despite being shipped, read as documentation, and run by the default pytest invocation. Both now cover `$(PATHS_PY)`. - The `tests-e2e` group asked for a `shiny[playwright]` extra py-shiny does not have, which uv reports as a warning rather than an error on every sync -- exactly how a real extras problem would hide. The remaining pre-existing errors in tests and examples are fixed in place: casts where a test deliberately passes `None` for an argument the handler ignores, narrowing helpers in `test_dep.py` for `HTMLDependency`'s wider-than-actual attribute types, and `.loc[:, ...]` in `04-shadcn` because pandas types `df[[...]]` as `Series | Unknown`. --- FEATURES.md | 18 ++---- examples/01-hello/app-core.py | 4 +- examples/01-hello/app.py | 4 +- examples/02-columns/app.py | 4 +- examples/03-columns-shadcn/app.py | 4 +- examples/11-npm-local/app.py | 4 +- pkg-py/src/shinyreact/_json.py | 28 --------- pkg-py/src/shinyreact/_reactive_output.py | 13 +--- pkg-py/src/shinyreact/_send_message.py | 8 +-- pkg-py/tests/test_reactive_output.py | 8 ++- pkg-py/tests/test_typing.py | 76 +++++++---------------- 11 files changed, 54 insertions(+), 117 deletions(-) delete mode 100644 pkg-py/src/shinyreact/_json.py diff --git a/FEATURES.md b/FEATURES.md index 230e33ec..dc4ed99b 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -192,7 +192,7 @@ R has no e2e suite, so no `(e2e)` leaf covers R (issue #194). - it accepts any JSON-serializable value - the client reads it with `useShinyOutputValue(id)` - call shape — **deliberate divergence** - - `[py]` a `Renderer[JsonValue]` subclass, used as a decorator, assigned + - `[py]` a `Renderer[Jsonifiable]` subclass, used as a decorator, assigned to `output[id]` - `[r]` a function, called as `output$id <- reactive_output(expr, ...)` - reason: each language's renderer idiom — a Python `Renderer` subclass has @@ -201,17 +201,11 @@ R has no e2e suite, so no `(e2e)` leaf covers R (issue #194). never a #184 parity question - `[py]` accepted types are whatever `Jsonifiable` admits: `dict`, `list`, `tuple`, `str`, `int`, `float`, `bool`, `None` - - spelled as `shinyreact._json.JsonValue`, whose containers are `Mapping` / - `Sequence` rather than `Jsonifiable`'s `dict` / `list` - - `dict` and `list` are **invariant** in their element types, so a render - function returning `dict[str, int]` is not assignable to `Jsonifiable` - — a type error at the most ordinary call site there is - - `Mapping` / `Sequence` are covariant, so `dict[str, int]`, - `dict[str, list[float]]` and `list[dict[str, str]]` all type-check with - no annotation at the call site - - same alias on `send_message(data=)`, for the same reason - - pinned by `pkg-py/tests/test_typing.py`, which asserts nothing at - runtime — pyright failing is the test + - a `dict` return does not **type-check** against `Jsonifiable`, whose + `dict` / `list` arms are invariant in their element types — upstream + py-shiny#2497, suppressed per call site with a + `# pyright: ignore[reportArgumentType]` naming the issue + - runtime behavior is unaffected; it is an annotation bug only - `[py]` a `str` return is sent as a JSON string, not a text node - `[py]` `auto_output_ui()` returns `None`, inherited from `Renderer` — there is no placeholder element to emit diff --git a/examples/01-hello/app-core.py b/examples/01-hello/app-core.py index bd2f128c..c2c5a6e5 100644 --- a/examples/01-hello/app-core.py +++ b/examples/01-hello/app-core.py @@ -4,7 +4,9 @@ def server(input: Inputs, output: Outputs, session: Session): - @reactive_output + # py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a + # `dict` return is not assignable to it. Drop the ignore when that lands. + @reactive_output # pyright: ignore[reportArgumentType] def dist_data(): return histogram(waiting, input.bins()) diff --git a/examples/01-hello/app.py b/examples/01-hello/app.py index ab8d2359..9a5293ed 100644 --- a/examples/01-hello/app.py +++ b/examples/01-hello/app.py @@ -5,7 +5,9 @@ set_react_page() -@reactive_output +# py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a +# `dict` return is not assignable to it. Drop the ignore when that lands. +@reactive_output # pyright: ignore[reportArgumentType] def dist_data(): return histogram(waiting, input.bins()) diff --git a/examples/02-columns/app.py b/examples/02-columns/app.py index 59b442cd..6888f090 100644 --- a/examples/02-columns/app.py +++ b/examples/02-columns/app.py @@ -25,6 +25,8 @@ def _handle_move(): columns.set(data) -@reactive_output +# py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a +# `dict` return is not assignable to it. Drop the ignore when that lands. +@reactive_output # pyright: ignore[reportArgumentType] def column_data(): return columns() diff --git a/examples/03-columns-shadcn/app.py b/examples/03-columns-shadcn/app.py index cee1cd7f..beca9383 100644 --- a/examples/03-columns-shadcn/app.py +++ b/examples/03-columns-shadcn/app.py @@ -22,7 +22,9 @@ def _handle_move(): data[to_col].append(item) columns.set(data) - @reactive_output + # py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a + # `dict` return is not assignable to it. Drop the ignore when that lands. + @reactive_output # pyright: ignore[reportArgumentType] def column_data(): return columns() diff --git a/examples/11-npm-local/app.py b/examples/11-npm-local/app.py index b0ca606f..8d44bb04 100644 --- a/examples/11-npm-local/app.py +++ b/examples/11-npm-local/app.py @@ -52,7 +52,9 @@ def histogram(values: list[float], bins: int) -> dict[str, list[float] | list[in def server(input: Inputs, output: Outputs, session: Session): - @reactive_output + # py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a + # `dict` return is not assignable to it. Drop the ignore when that lands. + @reactive_output # pyright: ignore[reportArgumentType] def dist_data(): return histogram(waiting, input.bins()) diff --git a/pkg-py/src/shinyreact/_json.py b/pkg-py/src/shinyreact/_json.py deleted file mode 100644 index df89a2ab..00000000 --- a/pkg-py/src/shinyreact/_json.py +++ /dev/null @@ -1,28 +0,0 @@ -from __future__ import annotations - -from typing import Mapping, Sequence, Union - -JsonValue = Union[ - str, - int, - float, - bool, - None, - Sequence["JsonValue"], - Mapping[str, "JsonValue"], -] -"""Any JSON-serializable value, as accepted from user code. - -Shiny's ``Jsonifiable`` spells its containers as ``list`` and ``dict``, both of -which are **invariant** in their element types. That makes the natural thing a -caller writes -- a function returning ``dict[str, int]``, or a -``dict[str, str]`` variable -- unassignable to it, so the most common -``reactive_output`` and ``send_message`` payloads fail to type-check for no -runtime reason. - -``Sequence`` and ``Mapping`` are covariant in their element types, so this -alias accepts exactly the values ``Jsonifiable`` describes without forcing -every call site to annotate its own return type as ``Jsonifiable``. Use it on -the way *in*, from user code; ``Jsonifiable`` stays the type on the way *out*, -to Shiny. -""" diff --git a/pkg-py/src/shinyreact/_reactive_output.py b/pkg-py/src/shinyreact/_reactive_output.py index 9c0e3893..5d041d5c 100644 --- a/pkg-py/src/shinyreact/_reactive_output.py +++ b/pkg-py/src/shinyreact/_reactive_output.py @@ -1,14 +1,10 @@ from __future__ import annotations -from typing import cast - from shiny.render.renderer import Renderer from shiny.types import Jsonifiable -from ._json import JsonValue - -class reactive_output(Renderer["JsonValue"]): +class reactive_output(Renderer["Jsonifiable"]): """Publish a reactive JSON value to the client (the ``ui.tsx`` pattern). Assign to ``output[id]`` where a React client reads the value with @@ -19,8 +15,5 @@ class reactive_output(Renderer["JsonValue"]): ``float``, ``bool``, ``None``), passed through unchanged. """ - async def transform(self, value: JsonValue) -> Jsonifiable: - # `JsonValue` and `Jsonifiable` describe the same runtime values; only - # their container variance differs, so this is a re-labeling, not a - # conversion. - return cast(Jsonifiable, value) + async def transform(self, value: Jsonifiable) -> Jsonifiable: + return value diff --git a/pkg-py/src/shinyreact/_send_message.py b/pkg-py/src/shinyreact/_send_message.py index 6b4252dd..1972beeb 100644 --- a/pkg-py/src/shinyreact/_send_message.py +++ b/pkg-py/src/shinyreact/_send_message.py @@ -1,11 +1,9 @@ from __future__ import annotations -from typing import TYPE_CHECKING, cast +from typing import TYPE_CHECKING from shiny.module import resolve_id -from ._json import JsonValue - if TYPE_CHECKING: from shiny.session import Session from shiny.types import Jsonifiable @@ -14,7 +12,7 @@ async def send_message( session: Session, id: str, - data: JsonValue, + data: Jsonifiable, ) -> None: """Send a custom message from server to client React components. @@ -38,5 +36,5 @@ async def notify(): """ namespaced_id = resolve_id(id) await session.send_custom_message( - "shinyReactMessage", {"id": namespaced_id, "data": cast("Jsonifiable", data)} + "shinyReactMessage", {"id": namespaced_id, "data": data} ) diff --git a/pkg-py/tests/test_reactive_output.py b/pkg-py/tests/test_reactive_output.py index e9ff6997..03ed6e98 100644 --- a/pkg-py/tests/test_reactive_output.py +++ b/pkg-py/tests/test_reactive_output.py @@ -36,7 +36,9 @@ def out(): @pytest.mark.asyncio async def test_passthrough_list() -> None: - @reactive_output + # py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a + # `dict` return is not assignable to it. Drop the ignore when that lands. + @reactive_output # pyright: ignore[reportArgumentType] def out(): return [1, 2, 3] @@ -53,7 +55,9 @@ def out(): def test_auto_output_ui_returns_none() -> None: - @reactive_output + # py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a + # `dict` return is not assignable to it. Drop the ignore when that lands. + @reactive_output # pyright: ignore[reportArgumentType] def my_value(): return {"x": 1} diff --git a/pkg-py/tests/test_typing.py b/pkg-py/tests/test_typing.py index ff57f010..21b8d096 100644 --- a/pkg-py/tests/test_typing.py +++ b/pkg-py/tests/test_typing.py @@ -1,17 +1,21 @@ """The public API as a *caller* sees it, checked by pyright rather than pytest. -These functions assert nothing at runtime -- they barely run. Their job is to -fail `make py-check-types` if a signature stops accepting the values user code -actually produces, which no runtime test can catch: an over-narrow annotation -still passes every assertion in `test_reactive_output.py`. - -Each return type below is written out deliberately. An unannotated -`def f(): return {"a": 1}` would be inferred, and inference is exactly what -hides the bug: `dict[str, int]` is not assignable to `dict[str, Jsonifiable]` -because `dict` is invariant in its value type, so the most ordinary thing a -`reactive_output` does used to be a type error at every call site (every -example app included). `Mapping` / `Sequence` are covariant, hence -`shinyreact._json.JsonValue`. +These functions assert nothing meaningful at runtime -- they barely run. Their +job is to fail `make py-check-types` if a signature stops accepting the values +user code actually produces, which no runtime test can catch: an over-narrow +annotation still passes every assertion in the rest of the suite. + +Return types are written out deliberately. An unannotated +`def f(): return {"a": 1}` would be inferred, and inference against the +parameter type is exactly what hides this class of bug. + +Not covered here, deliberately: the JSON payload types of `reactive_output` +and `send_message`. `Jsonifiable` spells its containers as `dict` and `list`, +both invariant in their element types, so a render function returning +`dict[str, int]` is not assignable to it -- the most ordinary thing a +`reactive_output` does. That is an upstream bug (py-shiny#2497), suppressed at +each call site with a `# pyright: ignore[reportArgumentType]` naming the issue; +grep `2497` to find them all when it lands. """ from __future__ import annotations @@ -19,47 +23,7 @@ from pathlib import Path from shiny import App, Inputs, Outputs, Session -from shinyreact import ReactApp, page_react_html, reactive_output, send_message - - -@reactive_output -def dict_of_int() -> dict[str, int]: - return {"a": 1} - - -@reactive_output -def dict_of_list() -> dict[str, list[float]]: - return {"breaks": [1.0, 2.0]} - - -@reactive_output -def nested_dict() -> dict[str, dict[str, list[str]]]: - return {"cols": {"A": ["Apple"]}} - - -@reactive_output -def list_of_dict() -> list[dict[str, str]]: - return [{"name": "a"}] - - -@reactive_output -def plain_str() -> str: - return "hello" - - -@reactive_output -def plain_none() -> None: - return None - - -async def send_typed_values(session: Session, counts: dict[str, int]) -> None: - """`send_message()` takes the same JSON values `reactive_output` returns. - - A dict *literal* would be inferred against the parameter type and pass - either way; a `dict[str, int]` variable is what catches the invariance. - """ - await send_message(session, "counts", counts) - await send_message(session, "names", ["a", "b"]) +from shinyreact import ReactApp, page_react_html def html_document_is_accepted_as_app_ui(index: Path) -> App: @@ -75,7 +39,9 @@ def server(i: Inputs, o: Outputs, s: Session) -> None: ... return App(page_react_html(index), server) -def react_app_is_a_shiny_app(index: Path) -> App: +def html_document_is_accepted_as_react_app_ui(index: Path) -> App: + """The same document through `ReactApp(ui=)`, which also mounts its dir.""" + def server(i: Inputs, o: Outputs, s: Session) -> None: ... return ReactApp(server, ui=page_react_html(index)) @@ -87,4 +53,4 @@ def test_module_type_checks() -> None: The real assertion is that pyright reports no error for this module; see `[tool.pyright] include` in `pyproject.toml`. """ - assert dict_of_int.auto_output_ui() is None + assert callable(html_document_is_accepted_as_app_ui) From 29ef380646add972b549cc3a11c61eea3cd1a526 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Sat, 12 Sep 2026 18:47:59 -0400 Subject: [PATCH 3/3] fix(py): import PageHtmlDocument type-only; fix ignore comment wording The private shiny.ui._page import is used only in a return annotation, so guard it under TYPE_CHECKING: an upstream rename becomes a pyright error instead of an ImportError at app startup. Also correct the py-shiny#2497 note on test_passthrough_list, which returns a list, not a dict. --- pkg-py/src/shinyreact/_page.py | 14 ++++++++------ pkg-py/tests/test_reactive_output.py | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg-py/src/shinyreact/_page.py b/pkg-py/src/shinyreact/_page.py index 0fb03ef6..f9f8676d 100644 --- a/pkg-py/src/shinyreact/_page.py +++ b/pkg-py/src/shinyreact/_page.py @@ -11,12 +11,6 @@ from shiny.session import get_current_session from shiny.ui import page_html -# The class `page_html()` returns. `App(ui=)` accepts it but not its -# `HTMLTextDocument` base, so declaring the base as our return type would make -# the documented `App(page_react_html(...), server)` fail to type-check. -# py-shiny exports the function but not the class -- hence the private import. -from shiny.ui._page import PageHtmlDocument - from ._app import SRC_DIR_ATTR from ._bookmark import _config_script_tag from ._dep import ShinyreactJs, _dep, _dep_page, _file_mtime_int, _serves_bundle @@ -25,6 +19,14 @@ # Private, but it is the only name for HTMLDependency's stylesheet entry. from htmltools._core import ScriptItem, StylesheetItem + # The class `page_html()` returns. `App(ui=)` accepts it but not its + # `HTMLTextDocument` base, so declaring the base as our return type would + # make the documented `App(page_react_html(...), server)` fail to + # type-check. py-shiny exports the function but not the class -- hence the + # private import, kept type-only so a rename upstream is a pyright error, + # not an ImportError at app startup. + from shiny.ui._page import PageHtmlDocument + def page_bare( *args: TagChild, diff --git a/pkg-py/tests/test_reactive_output.py b/pkg-py/tests/test_reactive_output.py index 03ed6e98..e4603018 100644 --- a/pkg-py/tests/test_reactive_output.py +++ b/pkg-py/tests/test_reactive_output.py @@ -37,7 +37,7 @@ def out(): @pytest.mark.asyncio async def test_passthrough_list() -> None: # py-shiny#2497: `Jsonifiable`'s `dict`/`list` arms are invariant, so a - # `dict` return is not assignable to it. Drop the ignore when that lands. + # `list` return is not assignable to it. Drop the ignore when that lands. @reactive_output # pyright: ignore[reportArgumentType] def out(): return [1, 2, 3]