diff --git a/docs/api-reference/browser_storage.md b/docs/api-reference/browser_storage.md index 59834ea3ed3..12d1fb26447 100644 --- a/docs/api-reference/browser_storage.md +++ b/docs/api-reference/browser_storage.md @@ -34,6 +34,18 @@ cannot be passed as a keyword argument — the keyword arguments (`name`, `max_age`, `sync`, etc.) only configure the storage behavior. ``` +```md alert info +# Declare browser storage as a default value or a storage annotation. + +A var is stored in the browser when its default is a storage value, such as +`theme: str = rx.LocalStorage("light", name="theme")`, or when it is annotated +with a storage type, such as `theme: rx.LocalStorage = "light"`. The storage key +and options must be known when the app compiles, so a `default_factory` cannot +provide them: `rx.field(default_factory=lambda: rx.LocalStorage(...))` fails to +compile on a var annotated with a storage type or with a type that accepts a +`str`, such as `str`, `str | None` or a `Literal` of strings. +``` + ```md alert warning # **The default value of a Cookie is never set in the browser!** diff --git a/docs/vars/base_vars.md b/docs/vars/base_vars.md index 8dc8b0adde2..c8dd7f6478c 100644 --- a/docs/vars/base_vars.md +++ b/docs/vars/base_vars.md @@ -106,7 +106,10 @@ storage var keeps its storage name and options only with a storage value as its default, such as `rx.LocalStorage("dark", name="theme")`. Given a plain string, a var annotated `str` becomes an ordinary var, and one annotated with a storage type, such as `rx.LocalStorage`, stays in browser storage under the default key -and options. +and options. A `default_factory` cannot declare browser storage: the app fails +to compile when a storage-typed var has a factory, or when the factory of a var +that accepts a `str` returns a storage value. See +[Browser Storage](/docs/api-reference/browser-storage/). An inherited var belongs to the state that declared it, so changing its field also changes the default for every state that inherits it. Each generated diff --git a/news/+reject-factory-storage-defaults.breaking.md b/news/+reject-factory-storage-defaults.breaking.md new file mode 100644 index 00000000000..cb5a06caa20 --- /dev/null +++ b/news/+reject-factory-storage-defaults.breaking.md @@ -0,0 +1 @@ +Browser storage vars (`rx.Cookie`, `rx.LocalStorage`, `rx.SessionStorage`) must be declared with a storage value as the default or with a storage type annotation. A `default_factory` on a storage var, or one returning a storage value, now fails at compile time instead of being silently ignored or treated as an ordinary var. diff --git a/reflex/compiler/utils.py b/reflex/compiler/utils.py index 8168fed9668..8490845954c 100644 --- a/reflex/compiler/utils.py +++ b/reflex/compiler/utils.py @@ -11,9 +11,10 @@ import tempfile import traceback from collections.abc import Iterable, Mapping, Sequence +from dataclasses import MISSING from datetime import datetime from pathlib import Path -from typing import TYPE_CHECKING, Any, TypedDict +from typing import TYPE_CHECKING, Any, Literal, TypedDict, get_args, get_origin from urllib.parse import urlparse from reflex_base import constants @@ -28,7 +29,8 @@ from reflex_base.constants.state import FIELD_MARKER from reflex_base.registry import RegistrationContext from reflex_base.style import Style -from reflex_base.utils import format, imports, memo_paths, serializers +from reflex_base.utils import format, imports, memo_paths, serializers, types +from reflex_base.utils.exceptions import VarTypeError from reflex_base.utils.imports import ImportVar, ParsedImportDict from reflex_base.vars.base import Field, Var, VarData from reflex_base.vars.function import DestructuredArg @@ -40,7 +42,12 @@ from reflex_components_core.el.elements.sectioning import Body from reflex.istate.delta import _resolve_delta -from reflex.istate.storage import Cookie, LocalStorage, SessionStorage +from reflex.istate.storage import ( + ClientStorageBase, + Cookie, + LocalStorage, + SessionStorage, +) from reflex.state import BaseState, cache_initial_snapshot from reflex.utils import path_ops from reflex.utils.exec import is_prod_mode @@ -326,17 +333,69 @@ def _compile_client_storage_field( if isinstance(field.default, field_type): cs_obj = field.default elif isinstance(field.type_, type) and issubclass(field.type_, field_type): - # A factory-backed field carries its options in what it produces. - produced = ( - field.default_factory() if field.default_factory is not None else None - ) - cs_obj = produced if isinstance(produced, field_type) else field.type_() + if field.default is MISSING: + _raise_factory_storage(field, field.type_) + cs_obj = field.type_() else: continue return field_type, cs_obj.options() + if ( + not field._backend + and field.default is MISSING + and field.default_factory is not None + and _may_hold_str(field.outer_type_) + ): + # A factory that only works at runtime declares no storage. + try: + produced = field.default_factory() + except Exception: + return None, None + if isinstance(produced, ClientStorageBase): + _raise_factory_storage(field, type(produced)) return None, None +def _may_hold_str(type_: Any) -> bool: + """Whether a value of the type can be a browser storage value, a str subclass. + + Args: + type_: The field's annotated type. + + Returns: + Whether the type accepts a str or a str subclass. + """ + type_ = types.resolve_type_alias(type_) + if type_ is Any: + return True + if types.is_union(type_): + return any(_may_hold_str(arg) for arg in get_args(type_)) + if get_origin(type_) is Literal: + return any(isinstance(arg, str) for arg in get_args(type_)) + return types.safe_issubclass(str, type_) or types.safe_issubclass(type_, str) + + +def _raise_factory_storage(field: Field, storage_type: type) -> None: + """Reject a browser storage default produced by a default_factory. + + The storage name and options must be known when the app compiles, so a + storage var declares them with a storage value as its default. + + Args: + field: The field with a default_factory. + storage_type: The browser storage type the field declares or produces. + + Raises: + VarTypeError: Always. + """ + owner = field._owner.__name__ if field._owner is not None else "" + msg = ( + f"Browser storage var '{owner}.{field._name}' cannot use a default_factory." + " Declare the storage as the default value instead, such as" + f' `{field._name}: str = rx.{storage_type.__name__}("", name={field._name!r})`.' + ) + raise VarTypeError(msg) + + def _compile_client_storage_recursive( state: type[BaseState], ) -> tuple[ diff --git a/tests/integration/tests_playwright/test_hydration_storage.py b/tests/integration/tests_playwright/test_hydration_storage.py index 0fbeb3914dd..7fbcc4f4d6f 100644 --- a/tests/integration/tests_playwright/test_hydration_storage.py +++ b/tests/integration/tests_playwright/test_hydration_storage.py @@ -79,9 +79,6 @@ class AssignedStorageState(rx.State): local: str = rx.LocalStorage("declared", name="assigned-local") session: str = rx.SessionStorage("declared", name="assigned-session") cookie: str = rx.Cookie("declared", name="assigned-cookie") - factory: rx.Field[str] = rx.field( - default_factory=lambda: rx.LocalStorage("declared", name="assigned-factory") - ) @rx.event def change(self): @@ -89,7 +86,6 @@ def change(self): self.local = "changed" self.session = "changed" self.cookie = "changed" - self.factory = "changed" fields = AssignedStorageState.__fields__ fields["local"].set_default(rx.LocalStorage("assigned", name="assigned-local")) @@ -97,8 +93,6 @@ def change(self): rx.SessionStorage("assigned", name="assigned-session") ) fields["cookie"].set_default(rx.Cookie("assigned", name="assigned-cookie")) - # A storage value as the default replaces the declared factory. - fields["factory"].set_default(rx.LocalStorage("assigned", name="assigned-factory")) class PreferenceState(rx.ComponentState): pref: str = rx.LocalStorage("declared", name="assigned-pref") @@ -233,7 +227,6 @@ def assigned(): rx.text(AssignedStorageState.local, id="assigned-local"), rx.text(AssignedStorageState.session, id="assigned-session"), rx.text(AssignedStorageState.cookie, id="assigned-cookie"), - rx.text(AssignedStorageState.factory, id="assigned-factory"), rx.button("Change", on_click=AssignedStorageState.change, id="change"), PreferenceState.create(initial="assigned"), rx.text(rx.cond(rx.State.is_hydrated, "true", "false"), id="hydrated"), @@ -375,7 +368,7 @@ def test_storage_annotations_hydrate_and_persist( # The browser storage vars of the assigned page, by element id suffix. -VARS = ("local", "session", "cookie", "factory", "pref") +VARS = ("local", "session", "cookie", "pref") def test_assigned_storage_default_persists( @@ -400,7 +393,6 @@ def test_assigned_storage_default_persists( localStorage.getItem('assigned-local') === 'changed' && sessionStorage.getItem('assigned-session') === 'changed' && localStorage.getItem('assigned-pref') === 'changed' && - localStorage.getItem('assigned-factory') === 'changed' && document.cookie.split('; ').includes('assigned-cookie=changed') """) diff --git a/tests/units/compiler/test_compiler_utils.py b/tests/units/compiler/test_compiler_utils.py index b92647a9677..d136ed8ac8a 100644 --- a/tests/units/compiler/test_compiler_utils.py +++ b/tests/units/compiler/test_compiler_utils.py @@ -2,6 +2,7 @@ import asyncio from copy import deepcopy +from typing import Annotated, Literal, TypedDict import pytest from reflex_base import constants @@ -18,6 +19,7 @@ from reflex.compiler.utils import write_file as compiler_write_file from reflex.constants.state import FIELD_MARKER from reflex.state import State, state_snapshot_hashes +from reflex.utils.exceptions import VarTypeError from reflex.utils.path_ops import write_file from reflex.vars.base import computed_var @@ -232,37 +234,153 @@ class MixedKeyState(State): assert len(state_snapshot_hashes(compiled)) == len(compiled) + 1 -def test_compile_client_storage_honors_default_factories( +@pytest.mark.parametrize( + "annotation", + [rx.Cookie, rx.LocalStorage, rx.SessionStorage], + ids=lambda t: t.__name__, +) +def test_compile_client_storage_rejects_factory_on_storage_annotation( + annotation: type, forked_registration_context: RegistrationContext, ): - """Factory-backed browser storage fields compile with the options they produce. + """A browser storage var annotated with a storage type takes no default_factory. + + Args: + annotation: The browser storage type the var is annotated with. + forked_registration_context: Keeps the test's state out of other tests. + """ + # Built with type() so the annotation is the parametrized storage type. + StorageState = type( + "StorageState", + (State,), + { + "__module__": __name__, + "__qualname__": "test_rejects_factory_on_storage_annotation..StorageState", + "__annotations__": {"value": annotation}, + "value": rx.field(default_factory=lambda: annotation("new", name="key")), + }, + ) + + with pytest.raises(VarTypeError, match=r"\.value' cannot use a default_factory"): + utils.compile_client_storage(StorageState) + + +def test_compile_client_storage_rejects_factory_on_set_default( + forked_registration_context: RegistrationContext, +): + """set_default with a default_factory is rejected on a browser storage var. Args: forked_registration_context: Keeps the test's state out of other tests. """ class StorageState(State): - cookie: rx.Field[rx.Cookie] = rx.field( - default_factory=lambda: rx.Cookie("new", name="cookie-key", max_age=60) + value: rx.LocalStorage = rx.LocalStorage("declared", name="storage-key") + + StorageState.__fields__["value"].set_default( + default_factory=lambda: rx.LocalStorage("new", name="storage-key") + ) + with pytest.raises(VarTypeError, match=r"\.value' cannot use a default_factory"): + utils.compile_client_storage(StorageState) + + +def test_compile_client_storage_rejects_factory_producing_storage( + forked_registration_context: RegistrationContext, +): + """A plain-annotated var whose default_factory produces browser storage is rejected. + + Args: + forked_registration_context: Keeps the test's state out of other tests. + """ + + class StorageState(State): + theme: rx.Field[str] = rx.field( + default_factory=lambda: rx.LocalStorage("light", name="theme") ) - local: rx.Field[rx.LocalStorage] = rx.field( - default_factory=lambda: rx.LocalStorage("new", name="local-key", sync=True) + + class OptionalStorageState(State): + theme: rx.Field[str | None] = rx.field( + default_factory=lambda: rx.LocalStorage("light", name="theme") ) - session: rx.Field[rx.SessionStorage] = rx.field( - default_factory=lambda: rx.SessionStorage("new", name="session-key") + + class OptionalAnnotatedStorageState(State): + theme: rx.Field[rx.LocalStorage | None] = rx.field( + default_factory=lambda: rx.LocalStorage("light", name="theme") ) - compiled = utils.compile_client_storage(StorageState) - name = StorageState.get_full_name() - cookie = compiled[constants.COOKIES][f"{name}.cookie{FIELD_MARKER}"] - assert (cookie["name"], cookie["maxAge"]) == ("cookie-key", 60) - assert compiled[constants.LOCAL_STORAGE][f"{name}.local{FIELD_MARKER}"] == { - "name": "local-key", - "sync": True, - } - assert compiled[constants.SESSION_STORAGE][f"{name}.session{FIELD_MARKER}"] == { - "name": "session-key" + class LiteralStorageState(State): + theme: rx.Field[Literal["light", "dark"]] = rx.field( # pyright: ignore[reportAssignmentType] + default_factory=lambda: rx.LocalStorage("light", name="theme") + ) + + class MetadataStorageState(State): + theme: rx.Field[Annotated[str, "meta"]] = rx.field( + default_factory=lambda: rx.LocalStorage("light", name="theme") + ) + + for state in ( + StorageState, + OptionalStorageState, + OptionalAnnotatedStorageState, + LiteralStorageState, + MetadataStorageState, + ): + with pytest.raises( + VarTypeError, match=r"\.theme' cannot use a default_factory" + ): + utils.compile_client_storage(state) + + +class Node(TypedDict): + """A typed dict default, which issubclass refuses to check.""" + + name: str + + +def test_compile_client_storage_skips_plain_factories( + forked_registration_context: RegistrationContext, +): + """A default_factory producing a plain value compiles as an ordinary var. + + Args: + forked_registration_context: Keeps the test's state out of other tests. + """ + calls = [] + + def make_text() -> str: + """Record the call and return a plain default. + + Returns: + The default text. + """ + calls.append(1) + return "plain" + + def missing_at_compile() -> str: + """Fail: a factory that only works at runtime is skipped while compiling. + + Raises: + RuntimeError: Always. + """ + msg = "only available at runtime" + raise RuntimeError(msg) + + class PlainState(State): + text: rx.Field[str] = rx.field(default_factory=make_text) + _token: rx.Field[str] = rx.field(default_factory=missing_at_compile) + runtime_only: rx.Field[str] = rx.field(default_factory=missing_at_compile) + items: rx.Field[list[str]] = rx.field(default_factory=list) + # issubclass against a TypedDict raises, so the check must not ask it. + node: rx.Field[Node] = rx.field(default_factory=lambda: Node(name="root")) + local: str = rx.LocalStorage("declared", name="local-key") + + compiled = utils.compile_client_storage(PlainState) + name = PlainState.get_full_name() + assert compiled[constants.LOCAL_STORAGE] == { + f"{name}.local{FIELD_MARKER}": {"name": "local-key", "sync": False} } + assert compiled[constants.COOKIES] == compiled[constants.SESSION_STORAGE] == {} + assert calls == [1] def test_document_root_allows_static_id_on_head_script(): diff --git a/tests/units/test_state.py b/tests/units/test_state.py index f326875462d..9dbfcb874b0 100644 --- a/tests/units/test_state.py +++ b/tests/units/test_state.py @@ -5982,47 +5982,6 @@ class StorageAnnotationState(State): assert declared.options() == options -def test_reset_client_storage_uses_declared_factories(): - """Hydration evaluates declared factories for all browser storage types.""" - calls = [] - - def factory(storage_type: type) -> Any: - """Record calls and return the configured browser default. - - Args: - storage_type: The browser storage value type. - - Returns: - The new storage value. - """ - calls.append(storage_type) - return storage_type("new") - - class StorageState(BaseState): - cookie: rx.Field[rx.Cookie] = rx.field( - default_factory=lambda: factory(rx.Cookie) - ) - local: rx.Field[rx.LocalStorage] = rx.field( - default_factory=lambda: factory(rx.LocalStorage) - ) - session: rx.Field[rx.SessionStorage] = rx.field( - default_factory=lambda: factory(rx.SessionStorage) - ) - - assert calls == [] - state = StorageState( - cookie=rx.Cookie("saved"), - local=rx.LocalStorage("saved"), - session=rx.SessionStorage("saved"), - ) - state._reset_client_storage() - assert state.cookie == state.local == state.session == "new" - assert calls == [rx.Cookie, rx.LocalStorage, rx.SessionStorage] - state._reset_client_storage() - assert state.cookie == state.local == state.session == "new" - assert calls == [rx.Cookie, rx.LocalStorage, rx.SessionStorage] * 2 - - @pytest.mark.parametrize( "breakage", [