[request a pair of eyes] experiments with history api about gloo HOT 8 CLOSED

One thought: You could expand the push and replace fns to include the full set of push_state_with_url args: ie data and title. Based on the multi-layered approach Gloo's taking, I think this could be a light, general wrapper used by routing APIs.

Related: This code:

fn history() -> web_sys::History {
    window().history().unwrap_throw()
}

fn window() -> web_sys::Window {
    web_sys::window().unwrap_throw()
}

, along with related things like Document, should be in a general low-level Gloo crate.

from gloo.

I agree with @David-OConnor that a push_with_state function would be good. Since the title is ignored in browsers (to my knowledge), we don't need to support it.

Similarly, it needs a way to retrieve the current state (from history.state)

This won't change the page, but will fire a popstate event.

Actually, no event is fired. The popstate event is only fired when moving through history, such as with back() or forward()

As far as I know, it is not possible to be notified when the URL changes with pushState or replaceState.

This is a major annoyance, but there's not too much we can do about it (other than creating our own event system, which might be a good idea!)

check_url

Is this function actually needed? The browser will already throw an exception if the URL is invalid. So you should be able to pattern match on that.

from gloo.

Do y'all think integrating serde serialization/deserialization of history state would be appropriate here? At a glance, it seems like something more appropriate for a routing (ie higher-level/opinionated) API, but perhaps it's useful to let history work directly with Rust structs, vice JsValue, which we'd hide.

from gloo.

I think accepting Into<JsValue> is reasonable. That also covers &str and String, which you can generate with serde_json.

I think direct support for Serde should be in a higher layer (such as a router).

from gloo.

I like that approach.

from gloo.

@David-OConnor

Do y'all think integrating serde serialization/deserialization of history state would be appropriate here?

My personal opinion is that there is definitely space for a routing library in gloo that is strongly typed, but that there will be several layers below this that need implementing first. Equivalent in JS would be history (low-level) -> react-router (higher-level). I think the low-level calls should just make the events nice to work with, and also fire the 'popstate' event on push/replace.

@Pauan

I agree with @David-OConnor that a push_with_state function would be good.

This design will need some iterations/improvements, at the moment I'm just getting it to work. Could you try running the test suite for wasm-history and see if you get the errors that I get? To reproduce

$ wasm-pack test --headless --firefox # takes aggggges but works
$ wasm-pack test --headless --chrome # crashes with very weird error
$ wasm-pack test --chrome # open chrom{e|ium} and navigate to tests to cause a crash

the headless chrome error I get is

Running headless tests in Chrome with `/usr/bin/chromedriver`
driver status: signal: 9
driver stdout:
    Starting ChromeDriver 72.0.3626.119 on port 35365
    Only local connections are allowed.
    Please protect ports used by ChromeDriver and related test frameworks to prevent access by malicious code.

error: invalid type: map, expected a string at line 1 column 68

So basically none of this output makes sense. 3626 is too big for an ipv4 address, it shouldn't be starting on a random ip anyway, what's being parsed at the bottom? I have no idea.

Maybe I should raise this with wasm-bindgen.

from gloo.

UPDATE The error is caused by OOM: (from cli logging)

[30:150:0408/153808.743127:FATAL:memory_linux.cc(42)] Out of memory.
[30:149:0408/153808.743124:FATAL:memory_linux.cc(42)] Out of memory.

from gloo.

Another update so I've changed the code to look for exceptions rather than parse the URL, and I'm no longer having problems. It all runs fast and I don't get any crashes. However, the problem I was experiencing here is clearly a bug somewhere. I'll raise it against wasm-bindgen to start with, but I'm not sure if it shouldn't be upstream in the browsers.

I will create a new issue when I've got a working API to get feedback (I'll incorporate what's been said in this thread).

from gloo.