Children.only can not be called on result of Children.map

[jimfb]

Children.only is intended to be called on an opaque Children object (eg. this.props.children). Best as I can tell, Children.map is intended to return an opaque Children object. But Children.only can't be called on the return value of Children.map, as per #4410 (comment)

It seems very reasonable to want to map your only child to another element (clone element, for instance), and then grab the only child from the map and use it to render. It is also very surprising for the identity operation to be passed into map, but the thing that comes out to not be semantically equivalent.

Issue demonstrated here: https://jsfiddle.net/9Ldyq5jk/4/

[sebmarkbage]

The intention of Children.only is to be used with the result of JSX where the child is guaranteed to be a single child. The reason for it is as an upgrade safety if JSX would change this behavior in the future.

[jimfb]

@sebmarkbage But isn't the intent of map that it return a data structure that's semantically equivalent to this.props.children? Specifically, the opaque Children data structure? Otherwise, isn't the return value of map completely useless? / how do you use it? What's the contract for the return type of map?

[sebmarkbage]

type SingleChild = ReactElement;
type SetOfChildren = Array<OpaqueChildren> | Map<string, OpaqueChildren>;
type OpaqueChildren = SingleChild | SetOfChildren;

JSX produces a children property:

createElement(type, props) : { props: {} }
createElement(type, props, child : SingleChild) : { props: { children : SingleChild } }
createElement(type, props, child : OpaqueChildren) : { props: { children : OpaqueChildren } }
createElement(type, props, child1 : SingleChild, child2 : SingleChild) : { props: { children : OpaqueChildren } }

only(children : OpaqueChildren) : SingleChild is a downcast that "warns" for the wrong type.

map(children : OpaqueChildren, fn) : OpaqueChildren doesn't promise to return anything that is compatible SingleChild. However, it is always compatible as a child of a div for example because it is compatible with createElement.

[jimfb]

Well, the documentation states:

Return the only child in children. Throws otherwise.

And since the signature is only(children : OpaqueChildren) (as per your comment above), it means that at the very least the documentation is wrong, since the function is not returning the only child in children.

I think the downcast behavior is unnecessarily complicated and incorrectly documented. At the very least we should fix the documentation, but my preferred solution is to fix the implementation. Regardless of our solution, I think this is a valid bug.

[sebmarkbage]

The point is that the whole thing is unnecessarily complicated since it is purely a perf optimization and an upgrade path if the perf optimization becomes unnecessary. However, people are likely relying on this.props.children being an element anyway, since we don't enforce it through a static type system. So I think that this whole utility is probably unnecessary.

[gaearon]

Sometimes, when I want a component to have no DOM of its own (and only have some lifecycle logic), I write

render() {
  return React.Children.only(this.props.children);
}

to enforce the one-childness ASAP. Otherwise I assume the error's going to be more cryptic to the consumer if they put several elements as children of my “container component”.

Am I misusing React.Children.only? Is it meant for a different use case?

[jimfb]

@sebmarkbage Ok, well, I've made my points here. I still feel like there is some sort of action item required (deprecate, fix, or document).

@gaearon It is my understanding that you are correctly using the React.Children.only helper in your example.

[LegNeato]

FWIW I just hit this and was about to file an issue as it seemed pretty broken to me too 🙊.

❤️ This code is straightforward and seems "correct" as far as the documentation is concerned (treating this.props.children as opaque and using React.Children to manipulate):

_renderInfo = (): any => {
  let infoChildren = React.Children.map(
    this.props.children,
    (child: ?Object): ?Object => {
      if (child.type.name === MyInfoComponent.name) {
        return child;
      }
    },
  );
  // If we have a MyInfoComponent child, render it. Throw if multiple, used wrong.
  if (React.Children.count(infoChildren) > 0) {
    return React.cloneElement(
      React.children.only(infoChildren),
      {
        hidden: this.state.infoHidden,
      },
    );
  }
};

It's a nice pattern to look for specific optional children and make sure there is only one of a certain type (so that siblings can be in any order). But doing it this way throws an error because of the one element array (hence this issue).

💩 This code looks worse to me as it is relying on the underlying implementation of this.props.children and has more lines...yet it works fine with no error:

_renderInfo = (): any => {
  let infoChildren = React.Children.map(
    this.props.children,
    (child: ?Object): ?Object => {
      if (child.type.name === MyInfoComponent.name) {
        return child;
      }
    },
  );
  // If we have a MyInfoComponent child, render it. Throw if multiple, used wrong.
 let numInfo = React.Children.count(infoChildren);
 if (numInfo > 1) {
   // throw
 } else if (numInfo === 1) {
    return React.cloneElement(
      infoChildren[0],
      {
        hidden: this.state.infoHidden,
      },
    );
  }
};

Note the replacement of React.children.only(infoChildren) with infoChildren[0] in cloneElement.

🍻 More than happy to put up a patch if there is consensus on what we should do (and if something should be done even).

[epsitec]

Shouldn't React.Children.only(x) be replaced by this function?

function universalChildrenOnly(children) {
  return (Array.isArray (children) && children.length === 1)
    ? children[0]
    : React.Children.only (children);
}

Still not sure what to do (see also #6156).

[sebmarkbage]

This is still helpful as a "safe" downcast for type systems so it's not great to deprecate it. However, it is also meant to effectively remove all runtime information. The runtime type check is not great to have for perf when you don't need it.

That runtime check is also not equivalent to React.Children.toArray(x)[0] because of how keys are preserved in nested arrays.

I think the fix is to fix the docs.