fix: fixed runtime panics in app shell

In some very specific HSR situations, the app shell could panic due to
holding a mutable borrow over an async boundary. That mutability is now
managed internally, which should remove that error (though replication
is almost impossible to remove).

packages/perseus/src/i18n/client_translations_manager.rs

@@ -3,17 +3,21 @@ use crate::errors::*;
 use crate::i18n::Translator;
 use crate::shell::fetch;
 use crate::utils::get_path_prefix_client;
+use std::cell::RefCell;
+use std::rc::Rc;
 
 /// Manages translations in the app shell. This handles fetching translations
 /// from the server as well as caching for performance. This is distinct from
 /// `TranslationsManager` in that it operates on the client-side rather than on
 /// the server. This optimizes for users viewing many pages in the same locale,
 /// which is by far the most common use of most websites in terms of i18n.
-#[derive(Debug)]
+///
+/// This holds mutability internally to avoid issues with async/await.
+#[derive(Debug, Clone)]
 pub(crate) struct ClientTranslationsManager {
     /// The cached translator. If the same locale is requested again, this will
     /// simply be returned.
-    cached_translator: Option<Translator>,
+    cached_translator: Rc<RefCell<Option<Translator>>>,
     locales: Locales,
 }
 impl ClientTranslationsManager {
@@ -22,24 +26,24 @@ impl ClientTranslationsManager {
     /// it can avoid network requests to unsupported locales.
     pub fn new(locales: &Locales) -> Self {
         Self {
-            cached_translator: None,
+            cached_translator: Rc::new(RefCell::new(None)),
             locales: locales.clone(),
         }
     }
     /// Gets an `&'static Translator` for the given locale. This will use the
     /// internally cached `Translator` if possible, and will otherwise fetch
-    /// the translations from the server. This needs mutability because it will
-    /// modify its internal cache if necessary.
+    /// the translations from the server. This manages mutability for caching
+    /// internally.
     pub async fn get_translator_for_locale<'a>(
-        &'a mut self,
+        &'a self,
         locale: &'a str,
-    ) -> Result<&'a Translator, ClientError> {
+    ) -> Result<Translator, ClientError> {
         let path_prefix = get_path_prefix_client();
         // Check if we've already cached
-        if self.cached_translator.is_some()
-            && self.cached_translator.as_ref().unwrap().get_locale() == locale
+        let mut cached_translator = self.cached_translator.borrow_mut();
+        if cached_translator.is_some() && cached_translator.as_ref().unwrap().get_locale() == locale
         {
-            Ok(self.cached_translator.as_ref().unwrap())
+            Ok(cached_translator.as_ref().unwrap().clone())
         } else {
             // Check if the locale is supported and we're actually using i18n
             if self.locales.is_supported(locale) && self.locales.using_i18n {
@@ -78,17 +82,17 @@ impl ClientTranslationsManager {
                     },
                 };
                 // Cache that translator
-                self.cached_translator = Some(translator);
+                *cached_translator = Some(translator);
                 // Now return that
-                Ok(self.cached_translator.as_ref().unwrap())
+                Ok(cached_translator.as_ref().unwrap().clone())
             } else if !self.locales.using_i18n {
                 // If we aren't even using i18n, then it would be pointless to fetch
                 // translations
                 let translator = Translator::new("xx-XX".to_string(), "".to_string()).unwrap();
                 // Cache that translator
-                self.cached_translator = Some(translator);
+                *cached_translator = Some(translator);
                 // Now return that
-                Ok(self.cached_translator.as_ref().unwrap())
+                Ok(cached_translator.as_ref().unwrap().clone())
             } else {
                 Err(ClientError::LocaleNotSupported {
                     locale: locale.to_string(),

packages/perseus/src/router/router_component.rs

@@ -9,7 +9,6 @@ use crate::{
     template::{RenderCtx, TemplateMap, TemplateNodeType},
     DomNode, ErrorPages, Html,
 };
-use std::cell::RefCell;
 use std::collections::HashMap;
 use std::rc::Rc;
 use sycamore::{
@@ -43,7 +42,7 @@ struct OnRouteChangeProps<'a, G: Html> {
     locales: Rc<Locales>,
     container_rx: NodeRef<G>,
     router_state: RouterState,
-    translations_manager: Rc<RefCell<ClientTranslationsManager>>,
+    translations_manager: ClientTranslationsManager,
     error_pages: Rc<ErrorPages<TemplateNodeType>>,
     initial_container: Option<Element>,
 }
@@ -189,7 +188,7 @@ pub(crate) fn perseus_router<G: Html>(
     // verison
     let container_rx = NodeRef::new();
 
-    let translations_manager = Rc::new(RefCell::new(ClientTranslationsManager::new(&locales)));
+    let translations_manager = ClientTranslationsManager::new(&locales);
     // Now that we've used the reference, put the locales in an `Rc`
     let locales = Rc::new(locales);
     // Get the error pages in an `Rc` so we aren't creating hundreds of them

packages/perseus/src/shell.rs

@@ -7,7 +7,6 @@ use crate::template::{PageProps, Template, TemplateNodeType};
 use crate::utils::get_path_prefix_client;
 use crate::ErrorPages;
 use fmterr::fmt_err;
-use std::cell::RefCell;
 use std::collections::HashMap;
 use std::rc::Rc;
 use sycamore::prelude::*;
@@ -270,7 +269,7 @@ pub(crate) struct ShellProps<'a> {
     pub router_state: RouterState,
     /// A *client-side* translations manager to use (this manages caching
     /// translations).
-    pub translations_manager: Rc<RefCell<ClientTranslationsManager>>,
+    pub translations_manager: ClientTranslationsManager,
     /// The error pages, for use if something fails.
     pub error_pages: Rc<ErrorPages<TemplateNodeType>>,
     /// The container responsible for the initial render from the server
@@ -351,10 +350,7 @@ pub(crate) async fn app_shell(
             checkpoint("page_visible");
 
             // Now that the user can see something, we can get the translator
-            let mut translations_manager_mut = translations_manager.borrow_mut();
-            // This gets an `Rc<Translator>` that references the translations manager,
-            // meaning no cloning of translations
-            let translator = translations_manager_mut
+            let translator = translations_manager
                 .get_translator_for_locale(&locale)
                 .await;
             let translator = match translator {
@@ -465,11 +461,7 @@ pub(crate) async fn app_shell(
                                 checkpoint("page_visible");
 
                                 // Now that the user can see something, we can get the translator
-                                let mut translations_manager_mut =
-                                    translations_manager.borrow_mut();
-                                // This gets an `Rc<Translator>` that references the translations
-                                // manager, meaning no cloning of translations
-                                let translator = translations_manager_mut
+                                let translator = translations_manager
                                     .get_translator_for_locale(&locale)
                                     .await;
                                 let translator = match translator {

packages/perseus/src/template/core.rs

@@ -258,12 +258,14 @@ impl<G: Html> Template<G> {
         &self,
         props: PageProps,
         cx: Scope<'a>,
-        translator: &Translator,
+        // Taking a reference here involves a serious risk of runtime panics, unfortunately (it's
+        // simpler to own it at this point, and we clone it anyway internally)
+        translator: Translator,
     ) -> View<G> {
         // The router component has already set up all the elements of context needed by
         // the rest of the system, we can get on with rendering the template All
         // we have to do is provide the translator, replacing whatever is present
-        provide_context_signal_replace(cx, translator.clone());
+        provide_context_signal_replace(cx, translator);
 
         (self.template)(cx, props)
     }