seed-rs / seed

A Rust framework for creating web apps
MIT License
3.81k stars 155 forks source link

0.4.1, initial page is incorrect #224

Closed craigmayhew closed 5 years ago

craigmayhew commented 5 years ago

As far as I can tell, the last good commit where this problem doesn't occur: c8b3b5d00897e7485e9c0aedbf9eb095d6eb8e96

I notice the example website also experiences this problem. If you view this url you see the home page but not the "events" page https://seed-rs.org/guide/3

David-OConnor commented 5 years ago

https://github.com/David-OConnor/seed-homepage/issues/6

Can repro. Thanks for IDing where it started. Fix is still WIP.

David-OConnor commented 5 years ago

I think the relevant change is here.

MartinKavik commented 5 years ago

It's by design, initial url should be handled in init method. Url is not handled in Seed's homepage: https://github.com/David-OConnor/seed-homepage/blob/master/src/lib.rs#L436. Example in RealWorld: https://github.com/MartinKavik/seed-rs-realworld/blob/master/src/lib.rs#L68.

It's in init because you can setup initial Model according to given url and you don't have to write an extra Model state/variant like Loading.

See PR https://github.com/David-OConnor/seed/pull/189 - 3. change

Inspired by Elm: https://package.elm-lang.org/packages/elm/browser/latest/Browser#application

David-OConnor commented 5 years ago

Got it working with this init function:

fn init(url: Url, orders: &mut impl Orders<Msg>) -> Model {
    let mut model = Model::default();
    update(routes(url).unwrap(), &mut model, orders);
    model
}

Is this the intended syntax for the trivial case? I may add some way around this, from an API perspective.

Homepage updated with the fix, but haven't updated the docs related to this yet.

MartinKavik commented 5 years ago

Is this the intended syntax for the trivial case?

No. That's why Elm has many "booting" functions: https://package.elm-lang.org/packages/elm/browser/latest/Browser.


The simplest / most explicit API I'm able to come up with now without big changes:

fn init(_: Url, _: &mut impl Orders<Msg>) -> Init {
    Init::new(Model::default(), UrlHandling::PassToRoutes) // or `UrlHandling::Handled`
}

As anonymous function:

#[wasm_bindgen]
pub fn render() {
    seed::App::build(|_, _| Init::new(Model::default(), UrlHandling::PassToRoutes), update, view)
        .routes(routes)
        .finish()
        .run();
}
David-OConnor commented 5 years ago

That API looks good to me. Did you already implement? If not, I'll take it.

Is your intent to import UrlHandling and Init in the prelude? Can you think of any other variants that it would have? If not, what do you think of having Init::New(Model::default()) be the syntax on its own? Could use builder pattern to add features later. An advantage is this is more concise for a common operation. A disadvantage is, if we make this more flexible later, it's less explicit.

MartinKavik commented 5 years ago

Did you already implement?

No.


Is your intent to import UrlHandling and Init in the prelude?

Yes.


Can you think of any other variants that it would have?

No. My first idea was just

seed::App::build(|_, _| (Model::default(), false), update, view)

where the second tuple value is url_handled. But it wasn't explicit enough so I've just rewritten it into more readable and autocomplete-friendly form.


what do you think of having Init::New(Model::default())

It seems to me a little bit too complex. If you want to use default url_handling we can add another constructor:

Init::new(Model::default())
Init::new_with_url_handling(Model::default(), UrlHandling::Handled)

And if we want to use builder pattern, then there is another option:

#[wasm_bindgen]
pub fn render() {
    seed::App::build(|_, _| Model::default(), update, view)
        .routes_without_initial_url(routes)
        .finish()
        .run();
}

A disadvantage is, if we make this more flexible later, it's less explicit.

Yeah, it's the reason why Rust still hasn't got default function arguments and why I suggested to add second parameter to Init::new so it force you to think about it when you are writing and reading code.

David-OConnor commented 5 years ago

Closed due to PR merge.