Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an Effect(Model, Msg) option to Update enum #52

Closed
jesskfullwood opened this issue Feb 19, 2019 · 9 comments
Labels
bug

Comments

@jesskfullwood
Copy link
Contributor

@jesskfullwood jesskfullwood commented Feb 19, 2019

My initial view of this lib is "cool, Elm but in Rust!". However it has also elements of React (componentDidMount etc) which reduce the "purity" of the experience for some convenience, which is obviously a design tradeoff.

Now in Elm, the update function returns a Model and a Msg. The model is immediately rendered by the view function, the Msg (if any) is added back into the event loop and is handled the next time round. This means data and effects flow in the same direction all the time, which IMO makes it nice to reason about.

The update function is seed is a little different. It only returns a Model, or rather, an Update<Model>. Any effects from the message (i.e. creating more messages) are, as far as I can tell, supposed to be handled 'in place', by passing around the App and running App.update as and when it is necessary.

I hit a 'race condition' of sorts using this style. I made a login fetch request, and when the function returns I want to redirect to the Home route. When fetch resolves it runs something like

|login_data| app.update(Msg::LoggedIn(login_data, app.clone()))

This callback triggers a new message to be sent through the event loop, OK, fine. Then I handle it in the update function like

match msg {       
   LoggedIn(session, app) => {
        app.update(Msg::ChangePage(Route::Home));
        Model { session: Some(session), ..model }
    },
    // ....
}

Spot the bug? app.update runs immediately, redirecting to Home, then returns to the update function, which creates a new model with the previous Route, effectively taking me back to the login page. I could change the new model to point to Route::Home too, but I really want all routing to happen only through Msg::ChangePage as that handles url-updating as well.

Really what I want is the Elm behavior of just returning a variant Effect(Model, Msg) from my match statement, and have the framework handle it in order. The I won't have to pass the App around, also.

Thoughts?

@David-OConnor David-OConnor added the bug label Feb 19, 2019
@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Feb 19, 2019

I never liked passing App around; would love to switch to the style you propose.

@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Feb 19, 2019

Check the latest commit. Basic functionality added, allowing chaining. Haven't found a way around passing the state/App around yet, but would ideally like to have a seed::update fn that can be called from anywhere.

Demo in counter ex

@jesskfullwood

This comment has been minimized.

Copy link
Contributor Author

@jesskfullwood jesskfullwood commented Feb 19, 2019

Very cool! I look forward to trying this out

@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Feb 20, 2019

Related, in earlier versions, the update function (user-created, not App.update) returned a model directly, allowing you to do recursive calls within it, ie:

match msg {       
   LoggedIn(session, app) => {
       Model { session: Some(session), ..update(Msg::ChangePage(Route::Home))}
    },
    // ....
}

This can still be done with some (verbose) unwrapping of the Update enum.

And, I'm suspicious I haven't fully addressed your original issue.

@jesskfullwood

This comment has been minimized.

Copy link
Contributor Author

@jesskfullwood jesskfullwood commented Feb 20, 2019

I tried it out and it tidied up lots of code which was very nice.

There is still a need to pass around the App in some places, because e.g. when the login button is clicked it sends a LoginSubmit message which needs the App to launch a fetch request.

LoginSubmit { data, app } => {
    fetch::spawn_local(login_request(data, app));
    Update::Skip(model)
}

One way around this would be to make fetch::spawn_local accept

Future<Item = Msg, Error = JsValue> + 'static

and the Msg could just be processed by app.update when it resolves (a la Elm) instead of needing to pass an App into the future. But I haven't looked at how this could be implemented.

Speaking in more general terms, it might make the execution model more consistent if each time app.update was called, it added the message to a queue, and then handled messages handled in FIFO order.

(BTW thanks for your hard work, think this framework has great potential)

@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Feb 20, 2019

Agree on that fetch approach. I'm going to add an Update unwrapper to the next commit, leaving us with three related patterns of using multiple messages in the update fn:

The example I posted above, modified to work with this new unwrapper. Updates the model from one message, then modifies it further with the current one, rendering after both changes are made.

match msg {       
   LoggedIn(session, app) => {
       Render(
           Model { session: Some(session), 
          ..update(Msg::ChangePage(Route::Home), model).model()}
       )
    },
    // ....
}

The Effect variant: Similar to above, but renders after each update.

match msg {       
   LoggedIn(session, app) => {
        Effect(
            Model { session: Some(session), ..model }, 
            Msg::ChangePage(Route::Home)
        )
    },
    // ....
}

Executing side effects, then updating based on a different message:

match msg {
    RoutePage(page) => {
        seed::push_path(vec![&page.to_string()]);
        update(Msg::ChangePage(page), model)
    },
}
@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Feb 22, 2019

What do you think aboutcalling it RenderThen instead of Effect ? Seems more descriptive.

WIP docs for this:

You can perform updates recursively, ie have one update trigger another. For example,
here's a non-recursive approach, where functions do_things() and do_other_things() each
act on an Model, and output a Model:

fn update(fn update(msg: Msg, model: Model) -> Update<Model> {
    match msg {
        Msg::A => Render(do_things(model)),
        // Update the model with do_things, then with do_other_things, then render.
        Msg::B => Render(do_other_things(do_things(model))),
    }
}

Here's a recursive equivalent. Note the use of the Update enum's model method, which returns
the model it wraps.

fn update(fn update(msg: Msg, model: Model) -> Update<Model> {
    match msg {
        Msg::A => Render(do_things(model)),
        // Update the model with do_things, then with do_other_things, then render.
        Msg::B => Render(do_other_things(update(Msg::A, model).model())),
    }
}

We can render the update, then chain another update using the RenderThen variant of Update:

fn update(fn update(msg: Msg, model: Model) -> Update<Model> {
    match msg {
        Msg::A => Render(do_things(model)),
        // Update the model with do_other_things, render, then update with do_things and render.
        Msg::B => RenderThen(
            do_other_things(model),
            Msg::A 
        )
    }
}
@David-OConnor

This comment has been minimized.

Copy link
Member

@David-OConnor David-OConnor commented Feb 25, 2019

Published as v0.2.9, and docs updated

@jesskfullwood

This comment has been minimized.

Copy link
Contributor Author

@jesskfullwood jesskfullwood commented Feb 26, 2019

I finally got around to trying out the API, and it feels like a nice improvement. I have some thoughts about how to stop passing App around; I will start a new issue for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.