Use App.bookmark_store to be the source of truth for session.bookmark.store; Remove setter for per-session bookmark store value

Author: schloerke

shiny/_app.py

@@ -31,7 +31,7 @@
 from ._error import ErrorMiddleware
 from ._shinyenv import is_pyodide
 from ._utils import guess_mime_type, is_async_callable, sort_keys_length
-from .bookmark import _globals as bookmark_globals
+from .bookmark._globals import BookmarkStore
 from .bookmark._restore_state import (
     RestoreContext,
     get_current_restore_context,
@@ -113,6 +113,12 @@ def server(input: Inputs, output: Outputs, session: Session):
     ui: RenderedHTML | Callable[[Request], Tag | TagList]
     server: Callable[[Inputs, Outputs, Session], None]
 
+    _bookmark_store: BookmarkStore
+
+    @property
+    def bookmark_store(self) -> BookmarkStore:
+        return self._bookmark_store
+
     def __init__(
         self,
         ui: Tag | TagList | Callable[[Request], Tag | TagList] | Path,
@@ -121,7 +127,8 @@ def __init__(
         ),
         *,
         static_assets: Optional[str | Path | Mapping[str, str | Path]] = None,
-        bookmarking: Optional[Literal["url", "query", "disable"]] = None,
+        # Document type as Literal to have clearer type hints to App author
+        bookmark_store: Literal["url", "server", "disable"] = "disable",
         debug: bool = False,
     ) -> None:
         # Used to store callbacks to be called when the app is shutting down (according
@@ -141,8 +148,8 @@ def __init__(
                 "`server` must have 1 (Inputs) or 3 parameters (Inputs, Outputs, Session)"
             )
 
-        if bookmarking is not None:
-            bookmark_globals.bookmark_store = bookmarking
+        self._bookmark_store = bookmark_store
+
         self._debug: bool = debug
 
         # Settings that the user can change after creating the App object.
@@ -363,7 +370,7 @@ async def _on_root_request_cb(self, request: Request) -> Response:
         request for / occurs.
         """
         ui: RenderedHTML
-        if bookmark_globals.bookmark_store == "disable":
+        if self.bookmark_store == "disable":
             restore_ctx = RestoreContext()
         else:
             restore_ctx = await RestoreContext.from_query_string(request.url.query)

shiny/bookmark/_bookmark.py

@@ -97,18 +97,20 @@ class Bookmark(ABC):
     This value could help determine how session state is saved. However, app authors will not be able to change how the session is restored as the server function will run after the session has been restored.
     """
 
+    # Making this a read only property as app authors will not be able to change how the session is restored as the server function will run after the session has been restored.
     @property
     def store(self) -> BookmarkStore:
-        # Should we allow for this?
-        # Allows a per-session override of the global bookmark store
-        if isinstance(self._session_root.bookmark._store, MISSING_TYPE):
-            return bookmark_globals.bookmark_store
-        return self._session_root.bookmark._store
+        """
+        App's bookmark store value
 
-    @store.setter
-    def store(self, value: BookmarkStore) -> None:
-        self._session_root.bookmark._store = value
-        self._store = value
+        Possible values:
+        * `"url"`: Save / reload the bookmark state in the URL.
+        * `"server"`: Save / reload the bookmark state on the server.
+        * `"disable"` (default): Bookmarking is diabled.
+        """
+
+        # Read from the App's bookmark store value.
+        return self._session_root.app.bookmark_store
 
     _proxy_exclude_fns: list[Callable[[], list[str]]]
     """Callbacks that BookmarkProxy classes utilize to help determine the list of inputs to exclude from bookmarking."""

shiny/bookmark/_globals.py

@@ -13,8 +13,5 @@
 bookmark_load_dir: BookmarkLoadDir | MISSING_TYPE = MISSING
 
 BookmarkStore = Literal["url", "server", "disable"]
-# TODO: Barret - Q: Should we have a `enable_bookmarking(store: BookmarkStore)` function?
-bookmark_store: BookmarkStore = "disable"
-
 
 # TODO: Barret; Q: I feel like there could be a `@shiny.globals.on_session_start` decorator that would allow us to set these values.