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

Consider making <> pure unwrapped fragment #11859

Closed
klimashkin opened this issue Dec 14, 2017 · 7 comments
Closed

Consider making <> pure unwrapped fragment #11859

klimashkin opened this issue Dec 14, 2017 · 7 comments

Comments

@klimashkin
Copy link

Feature request

Currently if we pass Fragment with children to any custom component, this component gets that fragment as a child

const App = props => (
  <MyComponent>
    <Fragment>
      <div/>
      <div/>
      <div/>
    </Fragment>
  </MyComponent>
);

const MyComponent = props => {
  Children.count(props.children); // 1

  Children.map(props.children, item => {
    console.log(item.type === Fragment); // true
  });
};

That means in all components where children is being altered (cloned) we need to check first that each item is a fragment first, and if it is - take its children instead of itself.

This was a little unexpected, because after reading the documentation the impression was that Fragment is a pure abstraction on the caller side, that Fragment doesn't propagate down to components, but only its children.

In the same time it's understandable because Fragment might have more supported props in the future that receiving component might want to read.

But fragment shortcut <> will not have props, it will always mean pure abstraction (to group its children to show them by condition, for instance).
So maybe react could unwrap its children right away when elements are created and pass them down as set of children (merge with another children on the same level)?

React 16.2

@amannn
Copy link
Contributor

amannn commented Dec 15, 2017

I've stumbled across exactly the same issue.

For this to work I now need a render function like this:

render() {
  const {children} = this.props;

  const resolvedChildren =
    children.type === Fragment
      ? children.props.children
      : children;

  const enhancedChildren = Children.map(resolvedChildren, child =>
    React.cloneElement(child, {onClick: this.onClick})
  );

  render enhancedChildren;
}

I'm not sure if that children.type === Fragment check is safe, same about the access to children.props.children. I was also expecting that a fragment is resolved by using Children.map and friends, so I wouldn't have to do this check.

@amannn
Copy link
Contributor

amannn commented Dec 15, 2017

As far as I understand, this part should be patched to also consider fragments for this to work:

if (Array.isArray(children)) {
for (let i = 0; i < children.length; i++) {
child = children[i];
nextName = nextNamePrefix + getComponentKey(child, i);
subtreeCount += traverseAllChildrenImpl(
child,
nextName,
callback,
traverseContext,
);
}
} else {

If that's the case, can I submit a PR?

@gaearon
Copy link
Collaborator

gaearon commented Dec 15, 2017

My understanding is this was an intentional decision. Paging @acdlite

@gaearon
Copy link
Collaborator

gaearon commented Jan 2, 2018

Also @clemmy might have context on why this behavior was chosen.

@clemmy
Copy link
Contributor

clemmy commented Jan 5, 2018

As you mentioned, the children traversal semantics for <Fragment></Fragment> treats it as an element. The <></> syntax was created to be shorthand for <Fragment></Fragment>, so with that as the pretext, the same semantics exist for <></> since it’s simply de-sugaring to <Fragment></Fragment>. With your proposed changes to the semantics, switching from <></> to <Fragment></Fragment> will no longer be a non-breaking change. I think it’s an interesting idea, but the fact that it’ll take a different component to implement (leading to more complexity), and breaks compatibility between <></> and <Fragment></Fragment> is not very compelling to me.

In response to @amannn, that patch will change the semantics for both <Fragment></Fragment> and <></>, which is a separate behaviour to what @klimashkin is requesting (only behavioural change for <></>).

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

I think changing behavior for both might be reasonable.
But we won't get much further in an issue here.
Please submit an RFC with your proposal: https://github.com/reactjs/rfcs

@arty-name
Copy link

I could not find an existing RFC on this subject so I have created one myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants