Change the app struct to be a real handle to an Yew app instance and make it possible to destroy a running app #1825

nicklaswj · 2021-04-27T11:30:34Z

I originally posted this as a discussion thread, however since I already have a working implementation that I think is ready - I thought it would be better in a PR form.

Motivation

Currently there is no way to unmount a Yew app after it has been started, which is useful/necessary if you want to use Yew to create standalone widget/components in a larger non-Yew project.

Our use case:
We are currently creating a Yew app that we want to distribute to our partners as a widget/plugin to be used on their own website.
Our current approach is to expose an initialization JS function that takes an html element which then mounts the yew app with App::mount_with_props, and HOPE that they don't call the initialization function twice. However we have found that this is pretty hard to control with single page websites, e.g. React website, especially when we don't control the surroundings of our Yew app. For this reason we would like to introduce a way to unmount/destroy an Yew app cleanly so we can expose the same functionality in our API.

Description

This PR have changed quite a bit with the input of @siku2 since I created it, so I thought it required a new description (I've kept the old one below).

The module app and the struct app::App has respectively been renamed app_handle and app_handle::AppHandle.
All the different public mount functions have been moved to root of the Yew create, and the already existing mount functions has changed signature to return an AppHandle. Specifically the following has been changed
- Signature change
  - start_app<COMP>() has changed to yew::start_app<COMP>() -> AppHandle<COMP>
  - start_app_with_props<COMP>(props: COMP::Properties) has changed to start_app_with_props<COMP>(props: COMP::Properties) -> AppHandle<COMP>
- Moved from App:: and changed signature
  - App::mount(self, element: Element) -> ComponentLink<COMP> has been moved and renamed to start_app_in_element<COMP>(element: Element) -> AppHandle<COMP>
  - App::mount_with_props(self, element: Element, props: COMP::Properties) -> ComponentLink<COMP> has been moved and renamed to start_app_with_props_in_element<COMP>(element: Element, props: COMP::Properties) -> AppHandle<COMP>
  - App::mount_as_body(self) -> ComponentLink<COMP> has been moved and renamed to start_app_as_body<COMP>() -> AppHandle<COMP>
  - mount_as_body_with_props(self, props: COMP::Properties) -> ComponentLink<COMP> has been moved an renamed to start_app_with_props_as_body<COMP>(props: COMP::Properties) -> AppHandle<COMP>
In addition to the above renaming of functions, all the mount functions now return an instance of app_handle::AppHandle. An AppHandle instance can only be created by mounting a yew application and it does not implement Clone, thus there can only be one AppHandle per mounted Yew application. The Apphandle implements one public function: AppHandle::destroy(self), which consumes the unique AppHandle and schedules the app's Scope to be destroyed.
AppHandle now implements Deref<Target = Scope<COMP>>, thus making it possible to get a reference to the app's scope and send messages to the app.

Unfortunately because we have changed the naming scheme of how to mount an Yew application, this PR now touches A LOT of files, e.g. all the yew examples. However all those changes are very trivial and mostly boils down to calling the same function but with a different name.

OLD Description

OLD Implementation proposal
Our proposal is to use underutilized yew::app::App struct by:

Change all the mount_* functions from mount(self) -> ComponentLink<..> to mount(self) -> self, i.e. store the resulting Scope in the App struct instead of returning it.
Add a method get_component_link(&self) -> ComponentLink<..> on App, to compensate for the changes in the function signatures in point 1.
Add a method destroy(self) on App that consumes the app and schedules a destruction of the inner scope of the app.

By doing the above we effectively change the App struct from being a "initialize and consume struct" to be a real handle to a specific instantiation of a Yew app.

Test
In the discussion post I promised to create new tests for this functionality, but I have not found any good way to test it.
If someone can point me in the right direction or have any ideas for one or more tests, I'll happily write them.
I have however created a new example called dyn_create_destroy_apps that showcases how to dynamically create and destroy instances of yew apps.

Checklist

I have run cargo make pr-flow
I have reviewed my own code
I have added tests
I have added an example

nicklaswj · 2021-04-27T12:51:33Z

This is of course a breaking change.

packages/yew/src/app.rs

examples/dyn_create_destroy_apps/index.scss

examples/dyn_create_destroy_apps/src/counter.rs

examples/dyn_create_destroy_apps/src/main.rs

nicklaswj · 2021-05-06T07:49:48Z

So @siku2 comment made me look at the App interface again, and I realize that with this PR you can mount an App with the same Scope multiple times, which isn't good.

I think the solution is to only return an instance of an App after it's mounted and remove App::new() from the public API.

siku2 · 2021-05-06T08:03:34Z

So @siku2 comment made me look at the App interface again, and I realize that with this PR you can mount an App with the same Scope multiple times, which isn't good.

I think the solution is to only return an instance of an App after it's mounted and remove App::new() from the public API.

Oh, you're totally right! How about we use #1652 and make the functions return a handle which can be used to destroy the app.
So basically move the mount stuff out of the App struct into a function-based API and rename it to AppHandle or something.

nicklaswj · 2021-05-06T08:15:57Z

So @siku2 comment made me look at the App interface again, and I realize that with this PR you can mount an App with the same Scope multiple times, which isn't good.
I think the solution is to only return an instance of an App after it's mounted and remove App::new() from the public API.

Oh, you're totally right! How about we use #1652 and make the functions return a handle which can be used to destroy the app.
So basically move the mount stuff out of the App struct into a function-based API and rename it to AppHandle or something.

That would basically be the same as the changes I'm doing now. If you give me 20 minutes then I have some code that we can discuss ;)

nicklaswj · 2021-05-06T08:26:39Z

So I have created my new suggestion for the public App interface here Skybox-Technologies@81fce4b
I didn't change the tests or examples yet, so I didn't want to invoke your CI on something I know would fail.

Basically the api is as follows:
You can only obtain an App instance by mounting it AND you can only mount an App by creating an App instance - hence no double mounting.

@siku2 this does pretty much what you suggest just with other function names. However I think it makes sense to keep the mounting/creating App fns under App (as this commit does)

siku2 · 2021-05-06T08:58:06Z

I do prefer the function interface for simplicity though. App is just a glorified namespace at this point.

yew::start_app::<Comp>();

// vs
yew::App::<Comp>::mount();

nicklaswj · 2021-05-06T09:05:13Z

I do prefer the function interface for simplicity though. App is just a glorified namespace at this point.
yew::start_app::<Comp>();

// vs
yew::App::<Comp>::mount();

Hmm. I don't agree. I look at the API as:

yew::start_app::<Comp>(); // Quick way and we don't care about getting a handle to the app

let app = yew::App::<Comp>::mount(); // Explicit way - we want to use the App instance for something.

But that said, I don't feel super strongly about it.

siku2 · 2021-05-06T09:05:17Z

BTW, since the destroy method really just forwards to Scope, do we really even need to keep the App struct around?
Seems to me like we could just return the scope and just properly document how to destroy it?

Right now I'm very inclined to just follow #1652

nicklaswj · 2021-05-06T09:06:47Z

BTW, since the destroy method really just forwards to Scope, do we really even need to keep it around?
Seems to me like we could just return the scope and just properly document how to destroy it?

Yes the App struct wraps the scope, so you can't call destroy twice on it (since it isn't Copy, unlike Scope). It makes for a nicer API IMO

nicklaswj · 2021-05-06T09:10:00Z

I've fixed the tests and examples, so I'll pull in the new branch to this PR (not because it's final in any way)

siku2 · 2021-05-06T09:12:17Z

Yes the App struct wraps the scope, so you can't call destroy twice on it (since it isn't Copy, unlike Scope). It makes for a nicer API IMO

That's a good point, I think it's fine to keep the handle around but I still want to simplify the mounting process.

Either we remove the public constructors from the handle altogether or we remove the convenience functions. Personally, I strongly believe that the functions are more convenient and look nicer.

nicklaswj · 2021-05-06T09:14:03Z

Yes the App struct wraps the scope, so you can't call destroy twice on it (since it isn't Copy, unlike Scope). It makes for a nicer API IMO

That's a good point, I think it's fine to keep the handle around but I still want to simplify the mounting process.

Either we remove the public constructors from the handle altogether or we remove the convenience functions. Personally, I strongly believe that the functions are more convenient and look nicer.

Fair enough. So we keep the functions and remove the public constructors. Do you want to rename App to something like AppHandle now that we are at it?

siku2 · 2021-05-06T09:18:18Z

Fair enough. So we keep the functions and remove the public constructors. Do you want to rename App to something like AppHandle now that we are at it?

Please do, yeah

nicklaswj · 2021-05-06T09:18:49Z

Fair enough. So we keep the functions and remove the public constructors. Do you want to rename App to something like AppHandle now that we are at it?

Please do, yeah

On it ;)

nicklaswj · 2021-05-06T09:29:56Z

Also, I introduced the function:

    /// Returns the app's component link.
    pub fn get_component_link(&self) -> ComponentLink<COMP> {
        self.scope.clone()
    }

Which I just realized probably should be substituted by impl AsRef instead, so you can do

let app_handle = yew::start_app::<Comp>();
app_handle.send_message(Comp::Message::Hello);

nicklaswj · 2021-05-06T09:44:50Z

@siku2 How strongly do you feel about the names of the functions

yew::start_app::<Comp>();`
// and 
start_app_with_props<COMP>(props: COMP::Properties)

They respectively call

App::<COMP>::mount_to_body()` 
// and
App::<COMP>::mount_to_body_with_props(props)

if we keep the names and want to provide the same functionality as App currently provides, then I can only come up with very awkward function names for the rest, the worst being the equivalent of mount_with_props(element: Element, props: COMP::Properties):

fn start_app_in_element_with_props(...) -> AppHandle

siku2 · 2021-05-06T09:56:09Z

@siku2 How strongly do you feel about the names of the functions
yew::start_app::<Comp>();`
// and 
start_app_with_props<COMP>(props: COMP::Properties)
They respectively call
App::<COMP>::mount_to_body()` 
// and
App::<COMP>::mount_to_body_with_props(props)
if we keep the names and want to provide the same functionality as App currently provides, then I can only come up with very awkward function names for the rest, the worst being the equivalent of mount_with_props(element: Element, props: COMP::Properties):
fn start_app_in_element_with_props(...) -> AppHandle

Feel free to change the names as you see fit.

The function get_component_link and destroy on App does not require Component::Properties to be Default. So move them to their own impl block.

Co-authored-by: Simon <simon@siku2.io>

* app.rs thorugh to public API: only make it possible to create an instance of an App by mounting it. And only make it possible to mount an App by creating it. This prevents double mounting. * Change Scope::mount_in_place to take &self instead of self, because it doesn't have too.

nicklaswj · 2021-05-17T08:59:05Z

Currently we call some global initialization code in AppHandle::new, which are directly or indirectly called by the yew::start_app*.
While it doesn't really create any problems, I think it's bad style to have such hidden side-effects. One solution is to resurrect the function yew::initialize again.
the init code is:

std::panic::set_hook(Box::new(console_error_panic_hook::hook));

examples/dyn_create_destroy_apps/README.md

siku2 · 2021-05-17T09:52:19Z

Currently we call some global initialization code in AppHandle::new, which are directly or indirectly called by the yew::start_app*.
While it doesn't really create any problems, I think it's bad style to have such hidden side-effects. One solution is to resurrect the function yew::initialize again.
the init code is:
std::panic::set_hook(Box::new(console_error_panic_hook::hook));

After thinking about it I think we should inline (get rid of) the new function since it isn't public and produces a semantically invalid AppHandle (the AppHandle doesn't actually own a valid scope).
The other functions can just use

let scope = Scope::new();
// ... do something
Self { scope }

which also looks cleaner in my opinion.

As for the panic hook, we should just move that to the yew::start_app functions. Specifically, only start_app_with_props_in_element, start_app_with_props, and start_app_with_props_as_body need to call it.

nicklaswj · 2021-05-17T09:59:35Z

Currently we call some global initialization code in AppHandle::new, which are directly or indirectly called by the yew::start_app*.
While it doesn't really create any problems, I think it's bad style to have such hidden side-effects. One solution is to resurrect the function yew::initialize again.
the init code is:
std::panic::set_hook(Box::new(console_error_panic_hook::hook));
After thinking about it I think we should inline (get rid of) the new function since it isn't public and produces a semantically invalid AppHandle (the AppHandle doesn't actually own a valid scope).
The other functions can just use
let scope = Scope::new();
// ... do something
Self { scope }
which also looks cleaner in my opinion.

Fair enough, I can do that.

As for the panic hook, we should just move that to the yew::start_app functions. Specifically, only start_app_with_props_in_element, start_app_with_props, and start_app_with_props_as_body need to call it.

That's doesn't really solve the underlying problem, since with this PR we make multiple concurrent Yew apps first class citizens and thus any global init code shouldn't be called multiple times just because you start multiple apps. However I'll do what you suggest, I just think we should have a note saying that this is maybe not the best approach.

packages/yew/src/lib.rs

packages/yew/src/app_handle.rs

Co-authored-by: Simon <simon@siku2.io>

siku2 · 2021-05-17T10:10:50Z

That's doesn't really solve the underlying problem, since with this PR we make multiple concurrent Yew apps first class citizens and thus any global init code shouldn't be called multiple times just because you start multiple apps. However I'll do what you suggest, I just think we should have a note saying that this is maybe not the best approach.

To me the only problematic part was that the hook was registered in a struct constructor instead of a global function since semantically a constructor shouldn't have visible side effects in the global scope.

There's nothing wrong with calling set_hook multiple times. The worst that could happen is that we overwrite a hook that the user manually registered beforehand.
The only solution I can think of is to not add the panic hook in start_app_with_props_in_element (because realistically that's the only one that should be called multiple times) and add back a public set_panic_hook() function which the user has to call manually.
Regardless, we should probably add a few words regarding this behaviour to the docstring.

nicklaswj · 2021-05-17T10:27:40Z

That's doesn't really solve the underlying problem, since with this PR we make multiple concurrent Yew apps first class citizens and thus any global init code shouldn't be called multiple times just because you start multiple apps. However I'll do what you suggest, I just think we should have a note saying that this is maybe not the best approach.

To me the only problematic part was that the hook was registered in a struct constructor instead of a global function since semantically a constructor shouldn't have visible side effects in the global scope.

There's nothing wrong with calling set_hook multiple times. The worst that could happen is that we overwrite a hook that the user manually registered beforehand.

Which could be a bit confusing for the user

The only solution I can think of is to not add the panic hook in start_app_with_props_in_element (because realistically that's the only one that should be called multiple times) and add back a public set_panic_hook() function which the user has to call manually.
Regardless, we should probably add a few words regarding this behaviour to the docstring.

I don't think we should make an asymmetric solution where we only set the panic hooks sometimes.

What if, we have a thread_local variable panic_hook_is_set: Cell<bool> that keeps track of if we have set the panic hook or not. In all the start_app* we do:

if !panic_hook_is_set.get() {
    std::panic::set_hook(Box::new(console_error_panic_hook::hook));
    panic_hook_is_set.set(true);
}

in addition we add a function:

pub fn set_custom_panic_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + Sync + Send + 'static>) {
    std::panic::set_hook(Box::new(console_error_panic_hook::hook));
    panic_hook_is_set.set(true);
}

So to summarize: If the panic hook is not set, we set it to what we usually do, however if it is set (either by the user or us) then we don't

siku2 · 2021-05-17T10:31:24Z

That's doesn't really solve the underlying problem, since with this PR we make multiple concurrent Yew apps first class citizens and thus any global init code shouldn't be called multiple times just because you start multiple apps. However I'll do what you suggest, I just think we should have a note saying that this is maybe not the best approach.

To me the only problematic part was that the hook was registered in a struct constructor instead of a global function since semantically a constructor shouldn't have visible side effects in the global scope.
There's nothing wrong with calling set_hook multiple times. The worst that could happen is that we overwrite a hook that the user manually registered beforehand.

Which could be a bit confusing for the user

The only solution I can think of is to not add the panic hook in start_app_with_props_in_element (because realistically that's the only one that should be called multiple times) and add back a public set_panic_hook() function which the user has to call manually.
Regardless, we should probably add a few words regarding this behaviour to the docstring.

I don't think we should make an asymmetric solution where we only set the panic hooks sometimes.

What if, we have a thread_local variable panic_hook_is_set: Cell<bool> that keeps track of if we have set the panic hook or not. In all the start_app* we do:
if !panic_hook_is_set.get() {
    std::panic::set_hook(Box::new(console_error_panic_hook::hook));
    panic_hook_is_set.set(true);
}
in addition we add a function:
pub fn set_custom_panic_hook(hook: Box<dyn Fn(&PanicInfo<'_>) + Sync + Send + 'static>) {
    std::panic::set_hook(Box::new(console_error_panic_hook::hook));
    panic_hook_is_set.set(true);
}
So to summarize: If the panic hook is not set, we set it to what we usually do, however if it is set (either by the user or us) then we don't

I like that solution!

This commit adds the function set_custom_panic_hook. In addition Yew will now check if a custom panic is registered when one of start_app* functions are called, if that is not the case a default panic hook is registered.

packages/yew/src/lib.rs

Co-authored-by: Simon <simon@siku2.io>

siku2

Thank you! I'm very excited about this change.

nicklaswj mentioned this pull request Apr 27, 2021

Deprecate App struct in favor of function based interface #1652

Closed

siku2 reviewed May 5, 2021

View reviewed changes

nicklaswj and others added 11 commits May 6, 2021 16:38

Naive implementation of destroy app

59d3ee4

Move functions get_component_link and destroy

b6ad5ea

The function get_component_link and destroy on App does not require Component::Properties to be Default. So move them to their own impl block.

Change the fn signatures for the other mount fns too

e9c8d6c

Add example of how to use the apps dynamically

3b3a387

Rename example

c98f936

Change title of example html file

db2ad55

Update examples/dyn_create_destroy_apps/src/counter.rs

d493caf

Co-authored-by: Simon <simon@siku2.io>

Fix examples to compile with new App signature

66ada30

Fix tests to compile with new App signature

039723f

Remove pub pulic App::mount* API

f4ff646

nicklaswj added 3 commits May 17, 2021 10:14

remove default impl for AppHandle

c341211

Make the tests compile again

5906f7e

Make the examples compile again

d49e291

siku2 reviewed May 17, 2021

View reviewed changes

examples/dyn_create_destroy_apps/README.md Show resolved Hide resolved

Add dyn_create_destroy_apps to examples/README.md

649bbeb

siku2 reviewed May 17, 2021

View reviewed changes

packages/yew/src/lib.rs Outdated Show resolved Hide resolved

packages/yew/src/app_handle.rs Outdated Show resolved Hide resolved

nicklaswj and others added 4 commits May 17, 2021 12:05

Remove mount_to_body_with_props

a7ff6f8

Co-authored-by: Simon <simon@siku2.io>

Use yew getters instead of query selectors

403f8a7

Co-authored-by: Simon <simon@siku2.io>

Remove AppHandle::new

6e543d1

Remove unused function

504dad8

nicklaswj added 2 commits May 17, 2021 12:11

code style fix

286c2af

Fix compile error

b600893

Add func set_custom_panic_hook

0029c82

This commit adds the function set_custom_panic_hook. In addition Yew will now check if a custom panic is registered when one of start_app* functions are called, if that is not the case a default panic hook is registered.

nicklaswj requested a review from siku2 May 17, 2021 11:30

siku2 reviewed May 17, 2021

View reviewed changes

packages/yew/src/lib.rs Outdated Show resolved Hide resolved

Fix docs for set_custom_panic_hook

fb03dae

Co-authored-by: Simon <simon@siku2.io>

siku2 added this to the v0.19 milestone May 17, 2021

siku2 added breaking change ergonomics feature-request A feature request labels May 17, 2021

siku2 approved these changes May 17, 2021

View reviewed changes

siku2 merged commit 09a41d6 into yewstack:master May 17, 2021

ranile removed this from the Upcoming release milestone Nov 24, 2021

voidpumpkin added the A-yew Area: The main yew crate label Nov 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the app struct to be a real handle to an Yew app instance and make it possible to destroy a running app #1825

Change the app struct to be a real handle to an Yew app instance and make it possible to destroy a running app #1825

nicklaswj commented Apr 27, 2021 •

edited

Loading

nicklaswj commented Apr 27, 2021

nicklaswj commented May 6, 2021 •

edited

Loading

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021 •

edited

Loading

siku2 commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021

nicklaswj commented May 6, 2021

nicklaswj commented May 6, 2021 •

edited

Loading

siku2 commented May 6, 2021

nicklaswj commented May 17, 2021

siku2 commented May 17, 2021

nicklaswj commented May 17, 2021

siku2 commented May 17, 2021

nicklaswj commented May 17, 2021

siku2 commented May 17, 2021

siku2 left a comment

Change the app struct to be a real handle to an Yew app instance and make it possible to destroy a running app #1825

Change the app struct to be a real handle to an Yew app instance and make it possible to destroy a running app #1825

Conversation

nicklaswj commented Apr 27, 2021 • edited Loading

Motivation

Description

OLD Description

Checklist

nicklaswj commented Apr 27, 2021

nicklaswj commented May 6, 2021 • edited Loading

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021 • edited Loading

nicklaswj commented May 6, 2021 • edited Loading

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021 • edited Loading

siku2 commented May 6, 2021 • edited Loading

nicklaswj commented May 6, 2021 • edited Loading

nicklaswj commented May 6, 2021

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021

siku2 commented May 6, 2021

nicklaswj commented May 6, 2021

nicklaswj commented May 6, 2021

nicklaswj commented May 6, 2021 • edited Loading

siku2 commented May 6, 2021

nicklaswj commented May 17, 2021

siku2 commented May 17, 2021

nicklaswj commented May 17, 2021

siku2 commented May 17, 2021

nicklaswj commented May 17, 2021

siku2 commented May 17, 2021

siku2 left a comment

Choose a reason for hiding this comment

nicklaswj commented Apr 27, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

siku2 commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading

nicklaswj commented May 6, 2021 •

edited

Loading