Unmounting by patch(vnode, null) documented but not implemented

[mreinstein]

My setup code:

const snabbdom = require('snabbdom')
const h = require('snabbdom/h').default; // helper function for creating vnodes


const patch = snabbdom.init([ // Init patch function with chosen modules
  require('snabbdom/modules/class').default, // makes it easy to toggle classes
  require('snabbdom/modules/props').default, // for setting properties on DOM elements
  require('snabbdom/modules/style').default, // handles styling on elements with support for animations
  require('snabbdom/modules/eventlisteners').default, // attaches event listeners
]);


let _currentVnode, _counter = 0;


const update = (oldVnode) => {
    const newVnode = h('div#container.two.classes', { on: { click: someFn } }, [
        h('span', { }, `counter: ${_counter}`)
    ]);

    _currentVnode = patch(oldVnode, newVnode);
};


const destroy = () => {
    patch(_currentVnode, null);
};


const someFn = (e) => {
    _counter++;
    update(_currentVnode);
    destroy();
};


const container = document.querySelector('div');
update(container);

When I click the div, I see the counter increment, but then there's a console error, and I never see the dom get unmounted:

bundle.js:177 Uncaught TypeError: Cannot read property 'key' of null

which corresponds to this code from snabbdom:

 function sameVnode(vnode1, vnode2) {
        return vnode1.key === vnode2.key && vnode1.sel === vnode2.sel;
    }

Any ideas what I'm doing wrong? Or is this a bug in the destroy patch code?

[mreinstein]

I'm using snabbdom@0.7.3 (installed from npm)

[mreinstein]

I've also tried the inline example listed in the readme, and this fails too, the same error: https://github.com/snabbdom/snabbdom#inline-example

I'm becoming more convinced this is either a bug or a malformed example. Any help would be greatly appreciated please. :)

[mightyiam]

Thank you for your interest in snabbdom, @mreinstein. I'm going to try to help you as much as I can.

I read your example code and I don't see what part the destroy function is meant to play in this example. Could you please try not calling destroy?

I'm going to close this but feel free to continue discussion.

[mreinstein]

I'm going to try to help you as much as I can.

I appreciate the willingness to help! Can you please not close the issue before we've considered it? I think once we look into this you'll see that something is going on.

I don't see what part the destroy function is meant to play

The purpose of destroy is to cause the snabbdom nodes to get removed from the web page. This is the advice that is provided in snabbdom's readme file. Search for to unmount from the DOM in the readme and you'll see this. Actually, even if you ignore my example code, and just try to run the inline example from the readme, you'll see the same exact error I'm encountering.

[mreinstein]

This example:

It has 2 problems:

• it references 2 callback functions that are undefined (someFn and anotherEventHandler don't exist anywhere in the example)
• after you define these 2 functions, you'll see the Uncaught TypeError: Cannot read property 'key' of null error I mentioned earlier in the console.

The first example in the readme is not usable.

[mightyiam]

It seems that the example is not a complete code example. Un-mounting is for the purpose of destroying the app. For example, if it's not a single page app, but only a part of a single page app (sub-app?), then before the parent app "navigates" away from that page, you should destroy the snabbdom sub-app. So it's not something you'd like to do as long as the app is running.

From what I've read, your example code seems like it should work fine without that call to destroy.

If you'd like to open a new issue regarding the documentation, please go ahead. If you'd like to submit a pull request to improve the documentation—even better. That would be appreciated.

[mreinstein]

Un-mounting is for the purpose of destroying the app

Yes, that is my understanding of it too. I am trying to completely unmount the application, permanently.
When I run the example that is listed in snabbdom's readme, it throws the error mentioned above, and does not unmount the dom element.

If you'd like to submit a pull request to improve the documentation—even better.

I am happy to submit a pull request to fix the documentation, but first I need to see a working example of patch(newVnode, null) to understand how it's used. Right now there are no working examples I can find of this. Can you please provide a working code sample, or fix the one listed above, so that I can resolve my issue and send the PR?

[mightyiam]

It seems that you're right and patch does not accept null as a second argument:

snabbdom/src/snabbdom.ts

Line 289 in 3f85807

return function patch(oldVnode: VNode | Element, vnode: VNode): VNode {

snabbdom/src/vnode.ts

Lines 13 to 20 in 3f85807

	export interface VNode {
	sel: string | undefined;
	data: VNodeData | undefined;
	children: Array<VNode | string> | undefined;
	elm: Node | undefined;
	text: string | undefined;
	key: Key | undefined;
	}

[nojvek]

Yeah I think we have to support null both ways.

Patch(null, vnode) should be equivalent of creating a nested document.createElement where you can get the vnode.elm and manually add it somewhere .e.g shadowRoot

And the opposite patch(vnode,elm) should completely destruct the tree by removing all event listeners and parent/child references such that the tree could be garbage collected by the js engine.

[mreinstein]

Patch(null, vnode) should be equivalent of creating a nested document.createElement

I'm not opposed to this, but it belongs in another ticket/issue as a feature request. This issue is specifically a bug report on the existing, broken, documented api for destroying an existing vnode and all of it's children.

[kay999]

I use something like

patch(old, h('!'))

to remove an element. It just patches a comment-node into old which effectively removes it. Seems a bit hackish and leaves a comment-node in the DOM but works for me.

[mreinstein]

I believe this ticket should be labelled as a bug. Also, I hate to be a pest but is there any update/thoughts on this?

[mightyiam]

On first look it seems like a non-breaking bug fix. Nothing to report.

[mreinstein]

ok, well thanks for the update anyway

[mreinstein]

@mightyiam I attempted a fix for this, via #536 Would this work?

[mightyiam]

Reviewing #536 I am asking myself what the return value of patch would be when provided null as a second argument. If it follows the current pattern of returning its second argument (right?) then it would be null. But that would be a breaking change because currently patch returns VNode:

snabbdom/src/snabbdom.ts

Line 291 in d4a8640

return function patch (oldVnode: VNode | Element, vnode: VNode): VNode {

In light of that, I suggest that this bug be fixed not by adding the missing implementation of the documented API, but by removing the documentation and coming up with a new, additional, API for unmounting a root VNode.

function unmount(vnode: VNode): void

Thinking about it, the only value of that seems to be the running of the remove and destroy hooks of that VNode tree.

Thoughts?

[mreinstein]

I think that's reasonable. If we do this we should document the workaround people are using as the "ordained" way of cleaning up ( patch(old, h('!')) )

[mreinstein]

#537 updates README.md as discussed.

That said, I think patch(old, null) is more intuitive than patch(old, h('!')) but at least this is an easy fix, and makes the docs reflect the current reality.