report better error for attrs that cannot be serialized (#1008)

* report better error for attrs that cannot be serialized

also misc changes to config options to make this eaiser
in particular ability to specify options as parents

* add more tests

* no need to be clever - just inherit

* fix tests

* ignore missing cov

* add test for non-json serializable attrs

* add changelog entry

* fix tests

* try node 16?

https://github.com/nodesource/distributions#using-ubuntu-3

* install npm explicitey

* try install in different order

* update first

* explicitely install npm i guess...

docs/Dockerfile

@@ -4,8 +4,8 @@ WORKDIR /app/
 
 # Install NodeJS
 # --------------
-RUN curl -sL https://deb.nodesource.com/setup_14.x  | bash -
-RUN apt-get install -yq nodejs build-essential
+RUN curl -fsSL https://deb.nodesource.com/setup_16.x | bash -
+RUN apt-get install -y build-essential nodejs npm
 RUN npm install -g npm@8.5.0
 
 # Install Poetry

docs/source/about/changelog.rst

@@ -23,7 +23,10 @@ more info, see the :ref:`Contributor Guide <Creating a Changelog Entry>`.
 Unreleased
 ----------
 
-No changes.
+**Fixed**
+
+- :issue:`930` - better traceback for JSON serialization errors (via :pull:`1008`)
+- :issue:`437` - explain that JS component attributes must be JSON (via :pull:`1008`)
 
 
 v1.0.0

src/py/reactpy/reactpy/_option.py

@@ -8,6 +8,7 @@ from reactpy._warnings import warn
 
 _O = TypeVar("_O")
 logger = getLogger(__name__)
+UNDEFINED = cast(Any, object())
 
 
 class Option(Generic[_O]):
@@ -16,8 +17,9 @@ class Option(Generic[_O]):
     def __init__(
         self,
         name: str,
-        default: _O | Option[_O],
+        default: _O = UNDEFINED,
         mutable: bool = True,
+        parent: Option[_O] | None = None,
         validator: Callable[[Any], _O] = lambda x: cast(_O, x),
     ) -> None:
         self._name = name
@@ -28,12 +30,15 @@ class Option(Generic[_O]):
         if name in os.environ:
             self._current = validator(os.environ[name])
 
-        self._default: _O
-        if isinstance(default, Option):
-            self._default = default.default
-            default.subscribe(lambda value: setattr(self, "_default", value))
-        else:
+        if parent is not None:
+            if not (parent.mutable and self.mutable):
+                raise TypeError("Parent and child options must be mutable")
+            self._default = parent.default
+            parent.subscribe(self.set_current)
+        elif default is not UNDEFINED:
             self._default = default
+        else:
+            raise TypeError("Must specify either a default or a parent option")
 
         logger.debug(f"{self._name}={self.current}")
 
@@ -81,11 +86,19 @@ class Option(Generic[_O]):
 
         Raises a ``TypeError`` if this option is not :attr:`Option.mutable`.
         """
+        old = self.current
+        if new is old:
+            return None
+
         if not self._mutable:
             msg = f"{self} cannot be modified after initial load"
             raise TypeError(msg)
-        old = self.current
-        new = self._current = self._validator(new)
+
+        try:
+            new = self._current = self._validator(new)
+        except ValueError as error:
+            raise ValueError(f"Invalid value for {self._name}: {new!r}") from error
+
         logger.debug(f"{self._name}={self._current}")
         if new != old:
             for sub_func in self._subscribers:
@@ -119,15 +132,23 @@ class Option(Generic[_O]):
         return f"Option({self._name}={self.current!r})"
 
 
-class DeprecatedOption(Option[_O]):  # nocov
-    def __init__(self, message: str, *args: Any, **kwargs: Any) -> None:
-        self._deprecation_message = message
+class DeprecatedOption(Option[_O]):
+    """An option that will warn when it is accessed"""
+
+    def __init__(self, *args: Any, message: str, **kwargs: Any) -> None:
         super().__init__(*args, **kwargs)
+        self._deprecation_message = message
 
     @Option.current.getter  # type: ignore
     def current(self) -> _O:
-        warn(
-            self._deprecation_message,
-            DeprecationWarning,
-        )
+        try:
+            # we access the current value during init to debug log it
+            # no need to warn unless it's actually used. since this attr
+            # is only set after super().__init__ is called, we can check
+            # for it to determine if it's being accessed by a user.
+            msg = self._deprecation_message
+        except AttributeError:
+            pass
+        else:
+            warn(msg, DeprecationWarning)
         return super().current

src/py/reactpy/reactpy/backend/_common.py

@@ -114,7 +114,7 @@ def vdom_head_elements_to_html(head: Sequence[VdomDict] | VdomDict | str) -> str
             head = cast(VdomDict, {**head, "tagName": ""})
         return vdom_to_html(head)
     else:
-        return vdom_to_html(html._(head))
+        return vdom_to_html(html._(*head))
 
 
 @dataclass

src/py/reactpy/reactpy/config.py

@@ -3,44 +3,65 @@ ReactPy provides a series of configuration options that can be set using environ
 variables or, for those which allow it, a programmatic interface.
 """
 
+from __future__ import annotations
+
 from pathlib import Path
 from tempfile import TemporaryDirectory
 
-from reactpy._option import Option as _Option
+from reactpy._option import Option
 
-REACTPY_DEBUG_MODE = _Option(
-    "REACTPY_DEBUG_MODE",
-    default=False,
-    validator=lambda x: bool(int(x)),
+TRUE_VALUES = {"true", "1"}
+FALSE_VALUES = {"false", "0"}
+
+
+def boolean(value: str | bool | int) -> bool:
+    if isinstance(value, bool):
+        return value
+    elif isinstance(value, int):
+        return bool(value)
+    elif not isinstance(value, str):
+        raise TypeError(f"Expected str or bool, got {type(value).__name__}")
+
+    if value.lower() in TRUE_VALUES:
+        return True
+    elif value.lower() in FALSE_VALUES:
+        return False
+    else:
+        raise ValueError(
+            f"Invalid boolean value {value!r} - expected "
+            f"one of {list(TRUE_VALUES | FALSE_VALUES)}"
+        )
+
+
+REACTPY_DEBUG_MODE = Option(
+    "REACTPY_DEBUG_MODE", default=False, validator=boolean, mutable=True
 )
-"""This immutable option turns on/off debug mode
+"""Get extra logs and validation checks at the cost of performance.
 
-The string values ``1`` and ``0`` are mapped to ``True`` and ``False`` respectively.
+This will enable the following:
 
-When debug is on, extra validation measures are applied that negatively impact
-performance but can be used to catch bugs during development. Additionally, the default
-log level for ReactPy is set to ``DEBUG``.
+- :data:`REACTPY_CHECK_VDOM_SPEC`
+- :data:`REACTPY_CHECK_JSON_ATTRS`
 """
 
-REACTPY_CHECK_VDOM_SPEC = _Option(
-    "REACTPY_CHECK_VDOM_SPEC",
-    default=REACTPY_DEBUG_MODE,
-    validator=lambda x: bool(int(x)),
-)
-"""This immutable option turns on/off checks which ensure VDOM is rendered to spec
-
-The string values ``1`` and ``0`` are mapped to ``True`` and ``False`` respectively.
-
-By default this check is off. When ``REACTPY_DEBUG_MODE=1`` this will be turned on but can
-be manually disablled by setting ``REACTPY_CHECK_VDOM_SPEC=0`` in addition.
+REACTPY_CHECK_VDOM_SPEC = Option("REACTPY_CHECK_VDOM_SPEC", parent=REACTPY_DEBUG_MODE)
+"""Checks which ensure VDOM is rendered to spec
 
 For more info on the VDOM spec, see here: :ref:`VDOM JSON Schema`
 """
 
-# Because these web modules will be linked dynamically at runtime this can be temporary
+REACTPY_CHECK_JSON_ATTRS = Option("REACTPY_CHECK_JSON_ATTRS", parent=REACTPY_DEBUG_MODE)
+"""Checks that all VDOM attributes are JSON serializable
+
+The VDOM spec is not able to enforce this on its own since attributes could anything.
+"""
+
+# Because these web modules will be linked dynamically at runtime this can be temporary.
+# Assigning to a variable here ensures that the directory is not deleted until the end
+# of the program.
 _DEFAULT_WEB_MODULES_DIR = TemporaryDirectory()
 
-REACTPY_WEB_MODULES_DIR = _Option(
+REACTPY_WEB_MODULES_DIR = Option(
     "REACTPY_WEB_MODULES_DIR",
     default=Path(_DEFAULT_WEB_MODULES_DIR.name),
     validator=Path,
@@ -52,7 +73,7 @@ assume anything about the structure of this directory see :mod:`reactpy.web.modu
 set of publicly available APIs for working with the client.
 """
 
-REACTPY_TESTING_DEFAULT_TIMEOUT = _Option(
+REACTPY_TESTING_DEFAULT_TIMEOUT = Option(
     "REACTPY_TESTING_DEFAULT_TIMEOUT",
     5.0,
     mutable=False,

src/py/reactpy/reactpy/core/serve.py

@@ -7,6 +7,7 @@ from typing import Callable
 from anyio import create_task_group
 from anyio.abc import TaskGroup
 
+from reactpy.config import REACTPY_DEBUG_MODE
 from reactpy.core.types import LayoutEventMessage, LayoutType, LayoutUpdateMessage
 
 logger = getLogger(__name__)
@@ -49,7 +50,18 @@ async def _single_outgoing_loop(
     layout: LayoutType[LayoutUpdateMessage, LayoutEventMessage], send: SendCoroutine
 ) -> None:
     while True:
-        await send(await layout.render())
+        update = await layout.render()
+        try:
+            await send(update)
+        except Exception:  # nocov
+            if not REACTPY_DEBUG_MODE.current:
+                msg = (
+                    "Failed to send update. More info may be available "
+                    "if you enabling debug mode by setting "
+                    "`reactpy.config.REACTPY_DEBUG_MODE.current = True`."
+                )
+                logger.error(msg)
+            raise
 
 
 async def _single_incoming_loop(

src/py/reactpy/reactpy/core/vdom.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-import logging
+import json
 from collections.abc import Mapping, Sequence
 from functools import wraps
 from typing import Any, Protocol, cast, overload
@@ -8,7 +8,7 @@ from typing import Any, Protocol, cast, overload
 from fastjsonschema import compile as compile_json_schema
 
 from reactpy._warnings import warn
-from reactpy.config import REACTPY_DEBUG_MODE
+from reactpy.config import REACTPY_CHECK_JSON_ATTRS, REACTPY_DEBUG_MODE
 from reactpy.core._f_back import f_module_name
 from reactpy.core.events import EventHandler, to_event_handler_function
 from reactpy.core.types import (
@@ -25,9 +25,6 @@ from reactpy.core.types import (
     VdomJson,
 )
 
-logger = logging.getLogger()
-
-
 VDOM_JSON_SCHEMA = {
     "$schema": "http://json-schema.org/draft-07/schema",
     "$ref": "#/definitions/element",
@@ -199,6 +196,8 @@ def vdom(
     attributes, event_handlers = separate_attributes_and_event_handlers(attributes)
 
     if attributes:
+        if REACTPY_CHECK_JSON_ATTRS.current:
+            json.dumps(attributes)
         model["attributes"] = attributes
 
     if children:
@@ -325,18 +324,18 @@ def _is_single_child(value: Any) -> bool:
 
 def _validate_child_key_integrity(value: Any) -> None:
     if hasattr(value, "__iter__") and not hasattr(value, "__len__"):
-        logger.error(
+        warn(
             f"Did not verify key-path integrity of children in generator {value} "
             "- pass a sequence (i.e. list of finite length) in order to verify"
         )
     else:
         for child in value:
             if isinstance(child, ComponentType) and child.key is None:
-                logger.error(f"Key not specified for child in list {child}")
+                warn(f"Key not specified for child in list {child}", UserWarning)
             elif isinstance(child, Mapping) and "key" not in child:
                 # remove 'children' to reduce log spam
                 child_copy = {**child, "children": _EllipsisRepr()}
-                logger.error(f"Key not specified for child in list {child_copy}")
+                warn(f"Key not specified for child in list {child_copy}", UserWarning)
 
 
 class _CustomVdomDictConstructor(Protocol):

src/py/reactpy/tests/test__option.py

@@ -33,7 +33,7 @@ def test_option_validator():
     opt.current = "0"
     assert opt.current is False
 
-    with pytest.raises(ValueError, match="invalid literal for int"):
+    with pytest.raises(ValueError, match="Invalid value"):
         opt.current = "not-an-int"
 
 
@@ -102,10 +102,36 @@ def test_option_subscribe():
 
 
 def test_deprecated_option():
-    opt = DeprecatedOption("is deprecated!", "A_FAKE_OPTION", None)
+    opt = DeprecatedOption("A_FAKE_OPTION", None, message="is deprecated!")
 
     with pytest.warns(DeprecationWarning, match="is deprecated!"):
         assert opt.current is None
 
     with pytest.warns(DeprecationWarning, match="is deprecated!"):
         opt.current = "something"
+
+
+def test_option_parent():
+    parent_opt = Option("A_FAKE_OPTION", "default-value", mutable=True)
+    child_opt = Option("A_FAKE_OPTION", parent=parent_opt)
+    assert child_opt.mutable
+    assert child_opt.current == "default-value"
+
+    parent_opt.current = "new-value"
+    assert child_opt.current == "new-value"
+
+
+def test_option_parent_child_must_be_mutable():
+    mut_parent_opt = Option("A_FAKE_OPTION", "default-value", mutable=True)
+    immu_parent_opt = Option("A_FAKE_OPTION", "default-value", mutable=False)
+    with pytest.raises(TypeError, match="must be mutable"):
+        Option("A_FAKE_OPTION", parent=mut_parent_opt, mutable=False)
+    with pytest.raises(TypeError, match="must be mutable"):
+        Option("A_FAKE_OPTION", parent=immu_parent_opt, mutable=None)
+
+
+def test_no_default_or_parent():
+    with pytest.raises(
+        TypeError, match="Must specify either a default or a parent option"
+    ):
+        Option("A_FAKE_OPTION")

src/py/reactpy/tests/test_config.py

@@ -27,3 +27,27 @@ def test_reactpy_debug_mode_toggle():
     # just check that nothing breaks
     config.REACTPY_DEBUG_MODE.current = True
     config.REACTPY_DEBUG_MODE.current = False
+
+
+def test_boolean():
+    assert config.boolean(True) is True
+    assert config.boolean(False) is False
+    assert config.boolean(1) is True
+    assert config.boolean(0) is False
+    assert config.boolean("true") is True
+    assert config.boolean("false") is False
+    assert config.boolean("True") is True
+    assert config.boolean("False") is False
+    assert config.boolean("TRUE") is True
+    assert config.boolean("FALSE") is False
+    assert config.boolean("1") is True
+    assert config.boolean("0") is False
+
+    with pytest.raises(ValueError):
+        config.boolean("2")
+
+    with pytest.raises(ValueError):
+        config.boolean("")
+
+    with pytest.raises(TypeError):
+        config.boolean(None)

src/py/reactpy/tests/test_core/test_vdom.py

@@ -280,28 +280,36 @@ def test_invalid_vdom(value, error_message_pattern):
         validate_vdom_json(value)
 
 
-@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only logs in debug mode")
-def test_debug_log_cannot_verify_keypath_for_genereators(caplog):
-    reactpy.vdom("div", (1 for i in range(10)))
-    assert len(caplog.records) == 1
-    assert caplog.records[0].message.startswith(
-        "Did not verify key-path integrity of children in generator"
-    )
-    caplog.records.clear()
+@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only warns in debug mode")
+def test_warn_cannot_verify_keypath_for_genereators():
+    with pytest.warns(UserWarning) as record:
+        reactpy.vdom("div", (1 for i in range(10)))
+        assert len(record) == 1
+        assert (
+            record[0]
+            .message.args[0]
+            .startswith("Did not verify key-path integrity of children in generator")
+        )
 
 
-@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only logs in debug mode")
-def test_debug_log_dynamic_children_must_have_keys(caplog):
-    reactpy.vdom("div", [reactpy.vdom("div")])
-    assert len(caplog.records) == 1
-    assert caplog.records[0].message.startswith("Key not specified for child")
-
-    caplog.records.clear()
+@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="Only warns in debug mode")
+def test_warn_dynamic_children_must_have_keys():
+    with pytest.warns(UserWarning) as record:
+        reactpy.vdom("div", [reactpy.vdom("div")])
+        assert len(record) == 1
+        assert record[0].message.args[0].startswith("Key not specified for child")
 
     @reactpy.component
     def MyComponent():
         return reactpy.vdom("div")
 
-    reactpy.vdom("div", [MyComponent()])
-    assert len(caplog.records) == 1
-    assert caplog.records[0].message.startswith("Key not specified for child")
+    with pytest.warns(UserWarning) as record:
+        reactpy.vdom("div", [MyComponent()])
+        assert len(record) == 1
+        assert record[0].message.args[0].startswith("Key not specified for child")
+
+
+@pytest.mark.skipif(not REACTPY_DEBUG_MODE.current, reason="only checked in debug mode")
+def test_raise_for_non_json_attrs():
+    with pytest.raises(TypeError, match="JSON serializable"):
+        reactpy.html.div({"non_json_serializable_object": object()})

src/py/reactpy/tests/test_utils.py

@@ -204,10 +204,6 @@ SOME_OBJECT = object()
             html.div(SOME_OBJECT),
             f"<div>{html_escape(str(SOME_OBJECT))}</div>",
         ),
-        (
-            html.div({"someAttribute": SOME_OBJECT}),
-            f'<div someattribute="{html_escape(str(SOME_OBJECT))}"></div>',
-        ),
         (
             html.div(
                 "hello", html.a({"href": "https://example.com"}, "example"), "world"