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

Triggering state changes as part of an on_visit handler #123

Open
murtyjones opened this issue Jun 2, 2019 · 4 comments
Open

Triggering state changes as part of an on_visit handler #123

murtyjones opened this issue Jun 2, 2019 · 4 comments

Comments

@murtyjones
Copy link
Contributor

murtyjones commented Jun 2, 2019

Hi!

I'd like to trigger a change to state (in this case, to hide an element) when a page is visited. To do so I put a .msg() in an on_visit handler, like so:

#[route(
  path = "/mypage",
  on_visit = hide_side_nav
)]
fn my_route(store: Provided<Rc<RefCell<Store>>>) -> VirtualNode {
    MyView::new(Rc::clone(&store)).render()
}

fn hide_side_nav(store: Provided<Rc<RefCell<Store>>>) {
    store.borrow_mut().msg(&Msg::HideSideNav);
}

Expected behavior

Page loads and the element I've specified is hidden as a result of the above message being sent

Actual behavior

This runtime error:

panicked at 'already borrowed: BorrowMutError'

Minimal Reproducible Example

In the isomorphic example, you can reproduce by changing download_contributers_json to send a message like so:

#[route(
  path = "/contributors",
  on_visit = download_contributors_json
)]
fn contributors_route(store: Provided<Rc<RefCell<Store>>>) -> VirtualNode {
    ContributorsView::new(Rc::clone(&store)).render()
}

fn download_contributors_json(store: Provided<Rc<RefCell<Store>>>) {
    store.borrow_mut().msg(&Msg::InitiatedContributorsDownload); // Arbitrary message
}

Notes

From reading a little bit about the error is that it's more of an Rc/RefCell problem than a percy problem. I thought would be useful to post it here anyways because I imagine others will get snagged on it at some point.

@chinedufn
Copy link
Owner

Yeah you're running into this:

// In order to check if the download has already been initiated, we must
// wrap the possibility of a download attempt in a closure and pass it to
// request_animation_frame. This is due to store already being mutably
// borrowed, since this method will be called from the `Store.msg` function.

Which was recently discovered by @austinsheep


I haven't yet thought about what changes in state management approach could fix this problem - so for now you might just use the same requestAnimationFrame strategy.

Open to any ideas you have of course!

I'll leave this issue open as this is definitely a problem that should be resolved!

@chinedufn
Copy link
Owner

More context

Basically on_visit gets called while Store is mutably borrowed so you can't just borrow it again.

pub fn msg(&mut self, msg: &Msg) {
match msg {
// TODO: Right now `on_visit` cannot borrow store since it's already borrowed.
// So we might want to explore wraping our `on_visit` in requestAnimationFrame
// so that by the time it runs we are no longer borrowing store ... or something ...
Msg::SetPath(path) => {
if let Some(router) = &self.router {
if let Some(route_handler) = router.matching_routerhandler(path.as_str()) {
route_handler.on_visit(path.as_str());
}

@murtyjones
Copy link
Contributor Author

Should've read the comments. Makes sense! Thank you

@chinedufn
Copy link
Owner

If you don't mind - going to keep this open until we land on a better way to handle this scenario which, like you said, other people will surely run into.

Thanks!

@chinedufn chinedufn reopened this Jun 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants