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...

Author: rmorshea

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 [email protected]
 
 # 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 @@
 
 _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 @@ def __init__(
         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 @@ def set_current(self, new: Any) -> None:
 
         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 @@ def __repr__(self) -> str:
         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 @@
 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)),
-)
-"""This immutable option turns on/off debug mode
+TRUE_VALUES = {"true", "1"}
+FALSE_VALUES = {"false", "0"}
 
-The string values ``1`` and ``0`` are mapped to ``True`` and ``False`` respectively.
 
-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``.
-"""
+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_CHECK_VDOM_SPEC = _Option(
-    "REACTPY_CHECK_VDOM_SPEC",
-    default=REACTPY_DEBUG_MODE,
-    validator=lambda x: bool(int(x)),
+REACTPY_DEBUG_MODE = Option(
+    "REACTPY_DEBUG_MODE", default=False, validator=boolean, mutable=True
 )
-"""This immutable option turns on/off checks which ensure VDOM is rendered to spec
+"""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:
 
-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.
+- :data:`REACTPY_CHECK_VDOM_SPEC`
+- :data:`REACTPY_CHECK_JSON_ATTRS`
+"""
+
+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 @@
 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 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,14 +1,14 @@
 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
 
 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 @@
     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)