VirtualNode renders arrays of children, but crashes hard trying to destroy them when removed.

[geekytime]

I bumped into this with some quick-and-dirty code to splat out an array of error messages.

In the example below, we can add an array of N virtual nodes as children by moving the slider, but when we drag the slider back to 0, and it tries to clean up the nodes, it fails with the error: Uncaught TypeError: c.destroy is not a function. After the error is thrown, it's like Yolk completely dies. Nothing in the UI responds after that.

var R = require("ramda");
var {render} = require("yolk");

var appEl = document.querySelector("#app");

var createChild = function(){
  return (<div>Child</div>);
};

var createChildren = function(event){
  return R.times(createChild, event.target.value);
};

var Component = function({createEventHandler}){
  var onChange$ = createEventHandler();

  var children$ = onChange$.map(createChildren);

  return (
    <div>
      <input onChange={onChange$} type="range" min="0" max="10" value="0" />
      <div>
        {children$}
      </div>
    </div>
  );
};

render((<Component/>), appEl);

It looks like this is the offending line of code: https://github.com/garbles/yolk/blob/master/src/VirtualNode.js#L110

[geekytime]

I can add a jsbin for this, I just wasn't sure if there was a recent Yolk dist available, as reported in #84.

[garbles]

Ack! I'm pretty sure I know what the issue is. Might get a chance to work on it tonight! Thanks for reporting

[brandonpayton]

It seems like the problem is that text children are strings (which have no destroy method) and that the solution is simply to stop calling destroy on string children.

I believe this is only an issue for VirtualNode because VirtualComponent's will be composed of VirtualNode's and never directly contain text children. Is that a correct understanding?

If it would help, I can submit a fix and a start on VirtualNode unit tests. Just let me know.

[garbles]

Yea, except that those text children end up getting wrapped in spans. Not sure whether I have to ensure that they're destroyed - concern about memory leaks.

[brandonpayton]

Ah, ok. It sounds like a proper destruction path is needed.

[brandonpayton]

We can move this conversation elsewhere if needed, but why is each text node wrapped in a span?

[garbles]

All text is wrapped in a span so that text could be indexed and moved around. I can just create a span and set the textContent to whatever the text was. I believe that this is what React used to do. It's easier than having to create a VirtualText class.

[brandonpayton]

Thanks! I have some follow up questions but can probably answer those by reading code.

[garbles]

@brandonpayton or open another issue. If something seems weird, it might be worth a second look.

[garbles]

I believe that recent changes have fixed this problem. Please rebase, let me know if it is persisting and open a PR with a failing spec!

[garbles]

Is this fixed now?

[jadbox]

Update: we're also seeing the TypeError: c.destroy is not a function when view components get removed.

[garbles]

If you write a test, I'll be able to fix it 😉

[jadbox]

I've submitted a PR that resolves the issue, but like the PR says, I'm not sure if this is a proper fix. It seems that if the jsx component only has a string child that the component destroy method receives the string as a child and tries to call destroy on it... which triggers the error.

[geekytime]

I'm also seeing this same error if I simply try to render into the same element twice. Probably because it's trying to remove the first one?

var Page1 = function(){
  return (<div>
    Page 1
  </div>);
};

var Page2 = function(){
  return (<div>
    Page 2
  </div>);
};

var appEl = document.querySelector(".app");
render(<Page1 />, appEl);
render(<Page2 />, appEl);

Page1 renders, but Page 2 throws the error.

This is a contrived example, but it's also how a simple location-based router would work. 😁