[request a pair of eyes] experiments with history api

[derekdreery]

I'm playing with wrapping the history api, which should be easy. I've got a crate, but the test in it takes ages to run on firefox, and full-on crashes chrome. It might be a bug in what I've done - but I would appreciate other people taking a look.

The code is currently called wasm-history.

[David-OConnor]

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.

[Pauan]

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.

[David-OConnor]

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.

[Pauan]

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).

[David-OConnor]

I like that approach.

[derekdreery]

@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.

[derekdreery]

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.

[derekdreery]

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).