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

React.cloneElement(this.props.children, this.props) copies the children into the this.props.children of children itself #7692

Closed
arshabh opened this issue Sep 10, 2016 · 4 comments

Comments

@arshabh
Copy link
Contributor

arshabh commented Sep 10, 2016

https://jsfiddle.net/69z2wepo/55774/

please run the js fiddle given above. cloneElement copies the children into props.children of the child itself

and if the child also renders its children using the same method - an infinite loop is established and the browser maximum call stack size is exceeded.

Ideally, while cloning the children property of the props of the child should be left untouched.

@gaearon
Copy link
Collaborator

gaearon commented Sep 10, 2016

This looks like correct behavior. children is part of props. If you pass this.props as second argument to cloneElement, children will be there. I don’t see why you’d expect them to be treated differently in this case, since children prop is not treated differently in any other cases.

@arshabh
Copy link
Contributor Author

arshabh commented Sep 11, 2016

Maybe we can provide a toggle or blacklist of props that we do not want to merge in such scenarios.

Here is my scenario. I am building an application using react, redux, and react-redux.
I have mapped my state values and action dispatch methods to props in my root component.

I have nested components in my application thanks to react-router. In order to pass state and dispatch methods as props to children i use

{ React.cloneElement(this.props.children, this.props) }

Now the issue is that when i use the same statement in a child to further pass down the pass state and dispatch methods as props - It renders itself which in turn again renders itself and so on - kicking in an infinite loop.

@gaearon
Copy link
Collaborator

gaearon commented Sep 11, 2016

I understand, but this is a very specific use case, and rather than bake it into React, I think it should be solved in userland. There are plenty of use cases where you want children to be passed so we will default to a less opinionated way, and it’s up to you to opt out and clarify what you meant with code.

For example, with spread operator:

const { children, ...otherProps } = this.props;
const newChild = React.cloneElement(children, otherProps);

Without object spread:

const { children } = this.props;

const otherProps = Object.assign({}, this.props);
delete otherProps.children;

const newChild = React.cloneElement(children, otherProps);

@arshabh
Copy link
Contributor Author

arshabh commented Sep 11, 2016

Sounds good to me. I agree with that my suggestion is opinionated based on my case. Thanks

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

2 participants