Skip to content

Mount takeover behavior concerns. #277

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

Open
AlterionX opened this issue Nov 3, 2019 · 3 comments
Open

Mount takeover behavior concerns. #277

AlterionX opened this issue Nov 3, 2019 · 3 comments
Labels
enhancement New feature or request

Comments

@AlterionX
Copy link
Contributor

The code in question is introduced in PR #276.

A new enum MountType was introduced for dealing with how to handle elements that are already attached to the root element (the mount_point). The behavior of MountType::Append is approximately the same as before the change. The behavior of MountType::Takeover is that it will add preexisting nodes to the tree, delete the underlying DOM nodes, then construct new DOM nodes.

The second is a little problematic. It is problematic because there can be nodes that should not be recreated (like script nodes). It also replaces unknown elements with the span element, which may also be problematic if someone has custom tags.

Additionally, the tag checking mechanism is slightly buggy, as the many tags have an upper case letter in their spellings, causing scripts to be removed from the DOM. The tag names are listing in dom_types.rs.

Lastly, this is also a bit inefficient, but it is a one time cost.

These issues might need to be addressed later.

@TatriX
Copy link
Member

TatriX commented Jan 14, 2020

Here's a reproducible example based on counter example from the latest master:

// ... snip

fn before_mount(_: Url) -> BeforeMount {
    BeforeMount::new()
        .mount_point(seed::body())
        .mount_type(MountType::Takeover)
}


#[wasm_bindgen(start)]
pub fn render() {
    seed::log!("Started");
    App::builder(update, view).before_mount(before_mount).build_and_start();
}

You will see "Started" printed 2 times, while it should be printed only once.

@MartinKavik
Copy link
Member

Warning about scripts in the mount point: https://github.com/seed-rs/seed/pull/334/files

@MartinKavik MartinKavik added the enhancement New feature or request label Mar 9, 2020
@MartinKavik MartinKavik added this to the 5. Optimizations milestone Mar 9, 2020
Ben-PH added a commit to Ben-PH/seed that referenced this issue May 26, 2020
Questions remain as to how this affects the concens in seed-rs#277
`rerender_vdom` and `bootstrap_vdom` may need more looking into
now that the `MountType` has been cleared.
Ben-PH added a commit to Ben-PH/seed that referenced this issue May 29, 2020
Questions remain as to how this affects the concens in seed-rs#277
`rerender_vdom` and `bootstrap_vdom` may need more looking into
now that the `MountType` has been cleared.
Ben-PH added a commit to Ben-PH/seed that referenced this issue May 29, 2020
Questions remain as to how this affects the concens in seed-rs#277
`rerender_vdom` and `bootstrap_vdom` may need more looking into
now that the `MountType` has been cleared.
MartinKavik pushed a commit to MartinKavik/seed that referenced this issue Jun 7, 2020
Questions remain as to how this affects the concens in seed-rs#277
`rerender_vdom` and `bootstrap_vdom` may need more looking into
now that the `MountType` has been cleared.
MartinKavik pushed a commit that referenced this issue Jun 8, 2020
Questions remain as to how this affects the concens in #277
`rerender_vdom` and `bootstrap_vdom` may need more looking into
now that the `MountType` has been cleared.
@Ben-PH
Copy link
Contributor

Ben-PH commented Dec 10, 2020

Does #478 close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants