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

Error handling for diff() using Result #104

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qknight
Copy link

@qknight qknight commented Jan 27, 2025

motivation

this PR adds Result<...> to diff() and was added to give users some feedback what went wrong when trying to create a diff on a vdom with concepts which are not supported. i.e. "

" two roots as in this example.

it works great if this two roots example is nested inside a div itself:

<div>
<div></div><div></div>
</div>

therefore the diff can now return InvalidRootNodeCount

details

Please have a look at:

  • SkipDiffError which is returned instead of vec![], not sure if this is wanted
  • also check the new InvalidRootNodeCount error, maybe this should actually be supported but is just broken in the recursive diff iteration
+        return Ok(vec![]);
+    }
+
+    // multiple root nodes are not supported. this would be two root nodes "<div></div><div></div>"
+    // the diff will work but the result is wrong, so instead we bail out here
+    match new_node {
+        Node::Leaf(leaf) => match leaf {
+            Leaf::NodeList(list) => {
+                if list.len() > 1 {
+                    log::error!("invalid root node cound: input needs exactly one root node and childs, not several root nodes");
+                    return Err(DiffError::InvalidRootNodeCount(list.len()));
+                }
+            }
+            _ => {}
+        },
+        Node::Element(_) => {}

motivation: a diff can't be computed for:

<div></div><div></div>

I don't know why but I made sure that the user who tries this gets a error message: InvalidRootNodeCount until it is implemented.

tests status

just test runs all tests successfully

prototype code remark

i've been playing with websocket auto-reconnect and article updates based on html generated from pandoc. so it works great and it is fast! thanks for this wonderful tool!

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

Successfully merging this pull request may close these issues.

1 participant