chinedufn / percy

Build frontend browser apps with Rust + WebAssembly. Supports server side rendering.
https://chinedufn.github.io/percy/
Apache License 2.0
2.26k stars 84 forks source link

Unreachable reached on virtual_dom_rs::patch::patch #62

Closed dbrgn closed 5 years ago

dbrgn commented 5 years ago

I'm applying a patch of a diff between the following nodes using the current master version of virtual-dom-rs:

img

Basically it's the change from <span><br></span> to <span>a<br></span>.

Unfortunately the trace only points to the WASM source:

img

Maybe you can reproduce this somehow in a test? I currently don't know how.

(By the way, the debug representation looks quite confusing to me. Maybe it could be made more easy to read?)

chinedufn commented 5 years ago

Thanks a lot for pointing this out!

Are you using console_error_panic_hook? If not that helps tremendously with the stack traces.

Maybe you can reproduce this somehow in a test? I currently don't know how.

So we typically just create a new diff_patch.rs test case and see if it fails.

https://github.com/chinedufn/percy/blob/bb394719b1fc277b99cf16c2fa070b7ebdec5c80/crates/virtual-dom-rs/tests/diff_patch.rs#L20-L33

wasm-pack test crates/virtual-dom-rs --firefox --headless --test diff_patch # Run browser tests

These tests make sure that you can diff and patch in both directions between two VirtualNode trees.

Basically it's the change from <span><br></span> to <span>a<br></span>.

I've been able to reproduce the issue thanks to your example

https://github.com/chinedufn/percy/blob/fix-62/crates/virtual-dom-rs/tests/diff_patch.rs#L163-L173

Will take a look (not tonight).

Thanks!!

chinedufn commented 5 years ago

FWIW another place that relates to this is the html macro tests

https://github.com/chinedufn/percy/blob/4a2770c8ff63dbe9e4faf383dcd58f7001e36952/crates/html-macro-test/src/lib.rs#L215-L228

But I took a look and the html macro is working fine so this looks like a diff or patch issue... I'll take a look

dbrgn commented 5 years ago

Are you using console_error_panic_hook? If not that helps tremendously with the stack traces.

Oh, that's nice, thanks for pointing it out!

chinedufn commented 5 years ago

Alright so I won't be able to take a look for a few days.

If you'd like to take a look at this in the meantime I'd be happy to answer any questions and/or point you towards where to look.

I've written up some documentation on the exact process to figure out what's going wrong.

https://chinedufn.github.io/percy/diff-patch/fixing-diff-patch-issues.html

image

chinedufn commented 5 years ago

Alright so I added a test case with your example (thanks!) and it does indeed fail!

https://github.com/chinedufn/percy/blob/cb41f080e31f5b84e88905c8b667d7f1e10d1626/crates/virtual-dom-rs/src/diff/mod.rs#L267-L280

image

Just to keep you posted, I should be able to take a look either tonight or within the next couple of days.

Cheers!

dbrgn commented 5 years ago

Great! I can probably give this a try tomorrow (unless you find time tonight).

chinedufn commented 5 years ago

@dbrgn so my screenshot above was totally wrong. The diffing was working fine I just had my Vec<Patch> in the test case backwards. The left and right in that screenshot have the save vector contents just in different orders...

Anyways.. the real issue was a patching issue.

The Patch::Replace(...) handler would treat text nodes like regular nodes so we were trying to create an element but we weren't providing a tag.. So basically calling document.create_element("");

https://github.com/chinedufn/percy/pull/65