[gloo_history::HashHistory] Assertion failed when calling HashHistory's location()

[andoalon]

Assertion failed when calling HashHistory's location()

HashHistory's location() asserts that what comes after the '#' in the URL starts with '/'. However the user could delete that '/', and if you then call location(), the assertion will trigger, saying "You cannot use relative path with this history type.".

In my specific case, I have a listener that changes the app's state accordingly when the user changes the URL. I do this using gloo_history::HashHistory. If the user inputs a URL with the wrong format, I cannot handle it using HashHistory.

Currently the only way I found to handle this is to first check the hash manually using gloo_history::BrowserHistory:

let is_bad = !gloo_history::BrowserHistory::new().location().hash().starts_with("#/");
// Avoid using `HashHistory` if `is_bad`

Steps to Reproduce

Option 1: manually

1. Create gloo_history::HashHistory
2. Change URL so that it ends with a '#' or contains something other than a '/' after the '#' (e.g. "http://127.0.0.1:8080/#aaa")
3. Call location() on gloo_history::HashHistory
4. See how an exception gets thrown due to the failed assertion, saying "You cannot use relative path with this history type."

Option 2: unit test

1. Open crates/history/tests/hash_history.rs
2. Go to the end of history_works()
3. Add the following code:

// Simulate the user changing the URL
window().location().set_hash("a").unwrap();

let location = history.location(); // <-- Exception thrown here
delayed_assert_eq(|| location.path().to_owned(), || "/").await;

4. Run the test with wasm-pack: wasm-pack test --firefox --headless crates/history/ --test hash_history (or similar)
5. See test fail due to the exception being thrown

Expected Behavior

Returning a location with empty path, maybe? Or making location() return Option so that this returns None? I'm not sure (suggestion are welcome). Definitely not an exception

Actual Behavior

Exception thrown

Additional Context

HashHistory tries to prevent this case from happening, but it does so during thread-local initialization, which only happens the first time you create a HashHistory:

// Hash needs to start with #/.
if current_hash.is_empty() || !current_hash.starts_with("#/") {
    let url = HashHistory::get_url();
    url.set_hash("#/");

    browser_history.replace(url.href());
}

This code does work as expected, except since the user can change the URL without triggering a page reload, the assumption that fixing the URL hash once will be enough is not actually true.

Not sure it's an ideal fix (since it is kinda lying about the true state of the URL), but the following patch makes the modified unit test I suggest in the "Steps to Reproduce" section ("Option 2: unit test") pass:

diff --git forkSrcPrefix/crates/history/src/hash.rs forkDstPrefix/crates/history/src/hash.rs
index 792b43a6074bac21d4f7d1b12475460a782a1a77..e050858fc8b4cbb8eb692349cea2ef7b4536618f 100644
--- forkSrcPrefix/crates/history/src/hash.rs
+++ forkDstPrefix/crates/history/src/hash.rs
@@ -196,18 +196,28 @@ impl History for HashHistory {
     fn location(&self) -> Location {
         let inner_loc = self.inner.location();
         // We strip # from hash.
-        let hash_url = inner_loc.hash().chars().skip(1).collect::<String>();
-
-        assert_absolute_path(&hash_url);
-
-        let hash_url = Url::new_with_base(
-            &hash_url,
-            &window()
-                .location()
-                .href()
-                .expect_throw("failed to get location href."),
-        )
-        .expect_throw("failed to get make url");
+        let hash_url = inner_loc.hash().strip_prefix('#').expect_throw("should not possible to not have the location hash start with '#'");
+
+        let hash_url = if !hash_url.starts_with('/') {
+            Url::new_with_base(
+                "",
+                &window()
+                    .location()
+                    .href()
+                    .expect_throw("failed to get location href."),
+            )
+            .expect_throw("failed to get make url")
+        } else {
+            Url::new_with_base(
+                hash_url,
+                &window()
+                    .location()
+                    .href()
+                    .expect_throw("failed to get location href."),
+            )
+            .expect_throw("failed to get make url")
+        };
 
         Location {
             path: hash_url.pathname().into(),

If you like this I could try submitting a PR, but I would like to first hear your thoughts. In any case, I feel that if I'm using HashHistory and using its API correctly (providing paths starting with '/', etc.), it shouldn't be possible for assertions to fail.