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

Replace mounted with rendered lifecycle method #1072

Merged
merged 2 commits into from Apr 25, 2020

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Apr 4, 2020

Problem

No callback for when a component re-renders

Changes

  • Remove mounted
  • Add rendered(&mut self, first_render: bool)
  • Update scheduler to eagerly destroy components to avoid unnecessary renders

Fixes: #1067
Unblocks: #1057

tag @ZainlessBrombie

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 4, 2020

I didn't add tests 😱 but @ZainlessBrombie you can start using this branch to make progress on use_context!

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 5, 2020

Excellent! :)

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 5, 2020

Uuh one of my old tests just started failing after merge D:
I have the scenario:
Component A has a child Component B:
In its view/render method, B sends a message to itself. When rendering A, it immediately also sends a message to itself that causes B to no longer be rendered. The message B sent never arrives, hence my use_effect is never called, but the surrounding code is. I'll try to reproduce

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 5, 2020

So I think the following test should not fail, but it does:

    #[wasm_bindgen_test]
    fn message_sent_if_component_destroyed() {
        #[derive(Properties, Clone)]
        struct UpdateCalledProps {
            update_called: Rc<dyn Fn()>,
        }
        struct MessageShouldArrive {
            link: ComponentLink<Self>,
            props: UpdateCalledProps,
        }

        impl Component for MessageShouldArrive {
            type Message = ();
            type Properties = UpdateCalledProps;

            fn create(props: Self::Properties, link: ComponentLink<Self>) -> Self {
                MessageShouldArrive { link, props }
            }

            fn update(&mut self, msg: Self::Message) -> bool {
                (self.props.update_called)();
                true
            }

            fn change(&mut self, _props: Self::Properties) -> bool {
                true
            }

            fn view(&self) -> Html {
                self.link.send_message(());
                return html! {
                    <div>{":)"}</div>
                };
            }
        }

        use std::cell::RefCell;
        struct MessageComponentWrapper {
            once: RefCell<bool>,
            link: ComponentLink<Self>,
            props: UpdateCalledProps,
        }

        impl Component for MessageComponentWrapper {
            type Message = ();
            type Properties = UpdateCalledProps;

            fn create(props: Self::Properties, link: ComponentLink<Self>) -> Self {
                MessageComponentWrapper {
                    once: RefCell::new(true),
                    link,
                    props,
                }
            }

            fn update(&mut self, msg: Self::Message) -> bool {
                true
            }

            fn change(&mut self, _props: Self::Properties) -> bool {
                true
            }

            fn view(&self) -> Html {
                if *self.once.borrow_mut().deref() {
                    *self.once.borrow_mut().deref_mut() = false;

                    self.link.send_message(());
                    return html! {
                        <MessageShouldArrive update_called=self.props.update_called.clone() />
                    };
                }
                return html! {
                    <div>{"done"}</div>
                };
            }
        }

        let app: App<MessageComponentWrapper> = yew::App::new();
        let destroy_counter = Rc::new(std::cell::RefCell::new(0));
        let destroy_country_c = destroy_counter.clone();
        app.mount_with_props(
            yew::utils::document().get_element_by_id("output").unwrap(),
            UpdateCalledProps {
                update_called: Rc::new(move || *destroy_country_c.borrow_mut().deref_mut() += 1),
            },
        );
        assert_eq!(1, *destroy_counter.borrow().deref());
    }

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 5, 2020

It only fails on this new branch

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 6, 2020

@ZainlessBrombie sounds like that is due to:

Update scheduler to eagerly destroy components to avoid unnecessary renders

This change will ignore update messages if the component has been destroyed

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 6, 2020

I see, makes sense. I was able to work around that, too.
Now there is one more thing:
I have the following setup.

<div>
    <ExampleContextProvider context=ExampleContext("wrong1".into())>
        <div>{"ignored"}</div>
    </ExampleContextProvider>
    <ExampleContextProvider context=ExampleContext("wrong2".into())>
        <ExampleContextProvider context=ExampleContext("correct".into())>
            <UseContextComponentInner />
        </ExampleContextProvider>
    </ExampleContextProvider>
    <ExampleContextProvider context=ExampleContext("wrong3".into())>
        <div>{"ignored"}</div>
    </ExampleContextProvider>
    <ExpectNoContextComponent />
</div>

ExpectNoContextComponent warns in the console if there is a context present. The context is removed in rendered()

The console contains the following log data:
PLACING HOOK
PLACING HOOK
PLACING HOOK
Context should be None here, but was ExampleContext("wrong3")!
PLACING HOOK
RECLAIMING HOOK
RECLAIMING HOOK
RECLAIMING HOOK
RECLAIMING HOOK
PLAING HOOK and RECLAIMING HOOK are logged in the ContextProvider impl

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 6, 2020

@ZainlessBrombie I see, I think we need each component to have its own scheduler. That's going to be a decent chunk of work but sounds fun :)

The problem is that we don't want to call rendered for a component until all of its updates have completed. We also don't want to update any component that is not a descendant of that component until we call rendered on it. That means we have to interleave updates tasks and rendered tasks and so the global scheduler isn't going to work.

I propose that the global scheduler has sub schedulers for each component and manages a tree structure of these sub schedulers to know which order to process them in.

Does that sound right?

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 6, 2020

but sounds fun :)

Lovely!

We also don't want to update any component that is not a descendant of that component until we call rendered on it

Exactly. Do you think managing a tree of components in the scheduler will be a performance concern?
Also, I was under the impression that the view() method is called width first not depth first right now, is that the case? If so having that be depth-first would be a prerequisite - and seems to be more predictable to me.

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 6, 2020

Do you think managing a tree of components in the scheduler will be a performance concern?

Hmm, I don't think it will be an issue.

Also, I was under the impression that the view() method is called width first not depth first right now, is that the case? If so having that be depth-first would be a prerequisite - and seems to be more predictable to me.

Yes, good point. It is breadth first right now, but should be depth first. That will be solvable with the tree approach as well

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 6, 2020

breadth right ^^
Ok. Sounds like we are good to go? :)

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 6, 2020

Ok. Sounds like we are good to go? :)

We have a plan! I won't have time for awhile to work on this though. You're welcome to give it a go if you have time!

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 6, 2020

If I find the time I'll try, but I'm currently working weekends because we are working on a project to coordinate delivery services for local stores so that might be some time off, idk yet ^^

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 6, 2020

Mmh I could imagine the scheduler having a "layer" counter and a stack of Schedulers. If the layer counter decreases it pops off the scheduler stack by that amount. And increments on push, of course

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 6, 2020

@ZainlessBrombie that sounds more important! We can revisit this later :)

Mmh I could image the scheduler having a "layer" counter and a stack of Schedulers. If the layer counter decreases it pops off the scheduler stack by that amount. And increments on push, of course

Yeah! A stack seems so natural for this. I think it runs into issues when one branch updates another though

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 6, 2020

I think it runs into issues when one branch updates another though

oh right yeah...
I'm wondering what happens if that update removes the subtree it was sent from... like we wouldn't even need to continue rendering then 🤔
I'll put my whiteboard back up haha

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 6, 2020

That's going to be a decent chunk of work but sounds fun :)

Like I said 😉

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 19, 2020

@ZainlessBrombie I've thought about this the past few days and I have come to the conclusion that it would be better if Yew remained more flexible in rendering and instead move the complexity into the context provider / subscription implementation.

Rationale:

Parents don't know when their children re-render

The context provider won't have the opportunity to shove its context in an accessible place for the children components if it never gets rendered. This means that it's the useContext hook's responsibility to search up the component tree for the appropriate context.

Strict rendering order will likely hinder future features

Right now, Yew can be pretty flexible in the areas of the component tree that it updates. This could allow us to do some cool async things, optimize diffing, etc. in the future.

@jstarry jstarry force-pushed the rendered branch 2 times, most recently from db02589 to 5e44991 Compare Apr 19, 2020
@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 19, 2020

Parents don't know when their children re-render

That is a very good point. I hadn't thought about that.
In that case useContext would need a way to inspect the component tree, which would A) require children to know their parents and B) may be a performance issue.
I wonder how react solves this, though to be fair they may have baked the hooks or at least Context right into the framework itself.

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 19, 2020

@jstarry
Copy link
Member Author

@jstarry jstarry commented Apr 20, 2020

Yeah, I was thinking that Yew would provide a way for each component to reference its parent. And each component would have a unique id. I think that's enough for the context implementation. When useContext is called, it would be called with the type so no need for reflection

@ZainlessBrombie
Copy link
Collaborator

@ZainlessBrombie ZainlessBrombie commented Apr 20, 2020

Yes that should work for a useContext implementation :)

@jstarry jstarry merged commit a91e7f6 into yewstack:master Apr 25, 2020
1 check passed
@jstarry jstarry deleted the rendered branch Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants