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

diff algorithm #105

Open
qknight opened this issue Jan 27, 2025 · 1 comment
Open

diff algorithm #105

qknight opened this issue Jan 27, 2025 · 1 comment

Comments

@qknight
Copy link

qknight commented Jan 27, 2025

I encountered an issue with a DOM/VDOM patch:

When trying to patch on line 14 the diff fails. The internals below show that dom::Node is correctly built from "

" but the resulting patch is wrong. I don't get what happens in the diff implementation, maybe this is a bug.

Interestingly I can patch to "" and then to something else again. But the diff needs one root node with childs, several root nodes is not working.

     1  #[wasm_bindgen_test]
     2  fn test_dom_vdom_patcher() {
     3      console_log::init_with_level(log::Level::Trace).ok();
     4      console_error_panic_hook::set_once();
     5      let id: String = "there".to_string();
     6
     7      let mut dom_updater: DomUpdater = DomUpdater::new(id.clone());
     8
     9      let html: String = "<div id=\"there\"></div>".to_string();
    10      dom_updater.update(html.clone());
    11      assert_eq!(html.to_string(), dom_updater.inner_html());
    12
    13      let html: String = "<div></div><div></div>".to_string();
    14      dom_updater.update(html.clone());
    15      assert_eq!(html, dom_updater.inner_html());
    16
    17      let html: String = "<div id=\"there\"><b>foo</b></div>".to_string();
    18      dom_updater.update(html.clone());
    19
    20      assert_eq!(html, dom_updater.inner_html());
    21  }

internals

 ]
    -------------------------------------------------
    -------------------------------------------------
    old_node: <div id="there"></div>
    inner_html: <div id="there"></div>
       => same
    new_node: <div></div><div></div>
    new_node: Leaf(
        NodeList(
            [
                Element(
                    Element {
                        namespace: None,
                        tag: "div",
                        attrs: [],
                        children: [],
                        self_closing: false,
                    },
                ),
                Element(
                    Element {
                        namespace: None,
                        tag: "div",
                        attrs: [],
                        children: [],
                        self_closing: false,
                    },
                ),
            ],
        ),
    )
    Created 1 VDOM patch(es)
    Created [Patch { tag: None, patch_path: TreePath { path: [] }, patch_type: ReplaceNode { replacement: [Element(Element { namespace: None, tag: "div", attrs: [], children: [Leaf(Text("boak"))], self_closing: false })] } }]
    Converted 1 DOM patch(es)
    Converted [DomPatch { patch_path: TreePath { path: [] }, target_element: DomNode { inner: Fragment }, target_parent: DomNode { inner: Fragment }, patch_variant: ReplaceNode { replacement: [DomNode { inner: Element { tag: "div", children: [None] } }] } }]

The source for this test is at https://github.com/ivanceras/sauron/pull/104/files#diff-cbb911e3744afc115b55c5d80223e0d486ef1f62dfbb5bcc6f06f4ef7038c75b

@ivanceras
Copy link
Owner

The library assumes there is only top-level root in the DOM. In cases where mutliple top-level root elements are used, the user can use fragment to wrap the elements into one top-level root. However, if there is a requirement to diff multiple top-level root elements, a good way to start is replacing the calls to diff function to diff-nodes instead. I don't have the bandwidth to do these changes at the moment, but if there is a PR that does this, without breaking the existing functionalities, then I would be happy to merge the PR.

@qknight qknight mentioned this issue Feb 7, 2025
59 tasks
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