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

DiffOpMove does not work in debug mode (only works in release mode) #955

Closed
Gronis opened this issue Apr 25, 2023 · 1 comment
Closed

DiffOpMove does not work in debug mode (only works in release mode) #955

Gronis opened this issue Apr 25, 2023 · 1 comment
Labels
bug Something isn't working

Comments

@Gronis
Copy link

Gronis commented Apr 25, 2023

While I was exploring the possibility of minimising the binary size of (my version of) the js-framework-benchmark for leptos framework, I ran into a problem where the "swap rows" button didn't work in debug mode. It turns out it only works in release mode. 😬

I have now found the issue which seems to be related to the function EachItem.get_opening_node which work differently in release and debug mode. In debug mode, the opening node is always a Comment type node. In release mode it's a normal Element type node. In debug mode, later when mount_child(MountKind::Before(&opening), &each_item); is called to insert the nodes at the right positions, it does not work because opening is a comment node. Changing the behavior of get_opening_node to work the same as release in mode fixes this problem.

Because I don't know the intent of having different implementations of get_opening_node, I decided not to open a pull request of my "fix". However, I still think this issue needs to be resolved, since a broken DiffOpMove implementation in debug builds are unacceptable.

This is the change I did to "fix" the issue:

leptos_dom/src/components.rs

impl Mountable for ComponentRepr {

    // ...

    fn get_opening_node(&self) -> web_sys::Node {
        // #[cfg(debug_assertions)]
        // return self._opening.node.clone();

        // #[cfg(not(debug_assertions))]
        return if let Some(child) = self.children.get(0) {
            child.get_opening_node()
        } else {
            self.closing.node.clone()
        };
    }

    // ...

}

leptos_dom/src/components/each.rs

impl Mountable for EachItem {

    // ...

    #[inline(always)]
    fn get_opening_node(&self) -> web_sys::Node {
        // #[cfg(debug_assertions)]
        // return self.opening.node.clone();

        // #[cfg(not(debug_assertions))]
        return self.child.get_opening_node();
    }

    // ...

}
@gbj gbj added the bug Something isn't working label Apr 26, 2023
@gbj
Copy link
Collaborator

gbj commented Apr 26, 2023

Thanks. The <For/> component is toward the tail end of a significant rewrite by @jquesada2016 (see #533, #897), which will likely resolve this issue in any case.

In the meantime, if you have a solution please do open a PR so we can test it.

Edit: Had my brain off for a second, I see your solution included in the issue here and have tested it and it does seem to work here. I'll test it against some other cases. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants