inferno-clone-vnode doesn't include children in cloned node

[johnsonw]

Inferno Metadata
OS: OS X
Browser: Chrome
Package: inferno-clone-vnode
version tested: 5.6.1 & 6.0.0-rc.5

Observed Behaviour
I am upgrading from inferno 3.x to 5.6.1 and as I was testing my codebase I came across an instance where a cloned node wasn't including its children. This behavior is different than what I was seeing in 3.x so I checked the docs to see if cloneVNode had changed:

Clone and return a new Inferno VNode using a VNode as the starting point. The resulting VNode will ?>have the original VNode's props with the new props merged in shallowly. New children will replace >existing children. key and ref from the original VNode will be preserved.

Expected Current Behaviour
Given that cloneVNode did not require the children to be passed as a third argument in previous version of Inferno, I feel like the children argument is optional and is intended to overwrite existing children, not empty the children completely, even if no children are passed in. Therefore, this ticket seeks to do the following:

1. Should cloneVNode behave the way it did in 3.x or is it expected that I should specify the children when calling it every time? My personal feeling is that I shouldn't have to specify the children as it seems redundant.
2. Show a test in which the children are not included in the cloned node:

import { render, Component } from "inferno";
import { cloneVNode } from "inferno-clone-vnode";

describe("cloneVNode", () => {
  let el;
  beforeEach(() => {
    class NameContainer extends Component {
      render() {
        const children = this.props.children.map(c =>
          cloneVNode(c, {
            name: "Henry"
          })
        );

        return <span>{children}</span>;
      }
    }

    const NameViewer = () => {
      return (
        <NameContainer>
          <div>
            <span>A child that should render after the clone</span>
          </div>
          <div>
            <span>A child that should render after the clone</span>
          </div>
        </NameContainer>
      );
    };

    el = document.createElement("div");
    document.body.appendChild(el);

    render(<NameViewer />, el);
  });

  it("should render", () => {
    expect(el).toMatchSnapshot();
  });
});

And here is the resulting snapshot:

exports[`cloneVNode should render 1`] = `
<div>
  <span>
    <div
      name="Henry"
    />
    <div
      name="Henry"
    />
  </span>
</div>
`;

Notice that the span children are not included. Now, if I update the NameContainer class as follows:

class NameContainer extends Component {
      render() {
        const children = this.props.children.map(c =>
          cloneVNode(c, {
            name: "Henry"
          }, c.children)
        );

        return <span>{children}</span>;
      }
    }

Let's look at the resulting snapshot:

exports[`cloneVNode should render 1`] = `
<div>
  <span>
    <div
      name="Henry"
    >
      <span>
        A child that should render after the clone
      </span>
    </div>
    <div
      name="Henry"
    >
      <span>
        A child that should render after the clone
      </span>
    </div>
  </span>
</div>
`;

Now the children are included. I tried putting together a fiddle but inferno-clone-vnode is not hosted on the cdn. The next best thing I could do is put together a jest test. Again, my personal opinion is that it is redundant to have to specify the children and this was not required in [email protected]. What are your thoughts? If you would like me to address the issue i'm willing to create a PR. Thanks for your time.

Regards,

Will

[Havunen]

I will have a look at this today

[johnsonw]

Thank you @Havunen. Let me know if I can help in any way.

[Havunen]

This issue reminds me of this one: https://github.com/infernojs/inferno/issues/1333

It was closed due to no steps to reproduce.

[johnsonw]

Yes, this issue does sound similar. I can get around it by passing the children from the element as the third argument (see the test I wrote above). Should we be expected to pass the children each time we call cloneVNode?

[Havunen]

@johnsonw lets cross verify from React how it works

[Havunen]

Yeah its bug

[johnsonw]

I created a fiddle with React showing that the children are rendered: https://jsfiddle.net/johnsonw/mc5o3a0g/1/

[johnsonw]

Let me know if you want me to follow up with a PR.

[Havunen]

@johnsonw If you want to fix it, sure! Test cases could be added here:
https://github.com/infernojs/inferno/blob/master/packages/inferno-clone-vnode/__tests__/cloneVNode.spec.jsx

I think same time when fixing that we could remove the rest parameter from children, otherwise its creating new array every time cloneVNode is called. Same pattern could be used as in createElement
https://github.com/infernojs/inferno/blob/master/packages/inferno-create-element/src/index.ts#L37-L45

Also we should add test case for scenario where children is provided as third parameter. And props missing or not missing.

[Havunen]

I converted your test case already to reproduce it in Inferno tests:

  describe("Issue 1393", () => {
    it("should render", () => {
      class NameContainer extends Component {
        render() {
          debugger;

          const children = this.props.children.map(c =>
            cloneVNode(c, {
              name: "Henry"
            })
          );

          return <span>{children}</span>;
        }
      }

      const NameViewer = () => {
        return (
          <NameContainer>
            <div className="test">
              <span>A child that should render after the clone</span>
            </div>
            <div>
              <span>A child that should render after the clone</span>
            </div>
          </NameContainer>
        );
      };

      const el = document.createElement("div");
      document.body.appendChild(el);

      render(<NameViewer />, el);

      expect(el.innerHTML).toBe('<span><div name="Henry"><span>A child that should render after the clone</span></div><div name="Henry"><span>A child that should render after the clone</span></div></span>')
    });
  });

[johnsonw]

@Havunen, no problem. I'll start working on this tonight 👍

[Havunen]

@johnsonw How is it going, can you get the PR done this weekend? I would like to proceed with v6 release, and this issue should be fixed

[johnsonw]

Hi @Havunen,

I've updated the source locally and the majority of my tests are passing now. I still have a few I am working through (not sure that they are related to this). Once all of the tests are passing i'm going to go through and do a manual test and then I should be able to commit the change. I might have it later tonight. Will keep you posted.

[johnsonw]

Quick update. With the change i've put in locally all of my tests are now passing. I'm now doing a manual test to make sure everything in our app is still working properly. Once I confirm that it is, I will start the PR process.

[johnsonw]

Hi @Havunen,

All tests are passing and i've added two additional tests. I've pushed the update to my local app and everything appears to be working correctly. Let me know if you have any questions and I will do my best to address them as quickly as possible.