Warn if people mutate children.

[jimfb]

Warn if people mutate children.

[jimfb]

I've seen this come up enough that we should just kill it with fire. cc @spicyj @gaearon

[sophiebits]

I've never seen this. You mean they're mutating the array that React.createElement makes? Can we just freeze it instead?

Public API so @facebook/react-core.

[jimfb]

createElement doesn't create an array; it just takes in an array and holds a reference to it, right? So no, I don't think we can just freeze the array. Best we can do is warn, I think.

But yeah, they're mutating the props.children array. It's terrible.

[sebmarkbage]

createElement does create an array when you use the var args. We could probably freeze that one. Needs to be tracked first if it is common though.

When people mutate their children what's the common pattern?

Do these gets stored in state with elements? That seems fine(ish). Probably used as a perf optimization by experts.

Or is it data stored in state which gets mutated by render to override it with elements?

I assume a common one is that a wrapper adds children to an existing set.

[jimfb]

I assume a common one is that a wrapper adds children to an existing set.

Yes, it is usually when a wrapper is mutating their props.children array.

I've also seen:

for(var i = 0; i < props.children.length; i++) {
   props.children[i] = <div>{props.children[i]}</div>;
}

I've also seen cases where the element is created, and elsewhere in the code (internally, UFI) there is logic to push elements into the array. It was a readability disaster of epic proportions.

[jimfb]

createElement does create an array when you use the var args. We could probably freeze that one.

Also, where is varargs? Signature of createElement is function(type, config, children).

[keyz]

https://github.com/facebook/react/blob/master/src/isomorphic/classic/element/ReactElement.js#L187

We could update the signature to function(type, config, ...children).

[jimfb]

ReactElement.js#L187

Ok, yeah, I'm fine with freezing that array. Either way, we probably want to warn for a release before breaking people's apps, right?

I don't really care, if you guys want to turn this into a breaking change, that's fine with me. Just let me know how to update the PR to move this forward.

[jimfb]

Ping @sebmarkbage, as per team sync.

[ghost]

@jimfb updated the pull request.

[syranide]

createElement doesn't create an array; it just takes in an array and holds a reference to it, right? So no, I don't think we can just freeze the array. Best we can do is warn, I think.

Even if it takes an array I don't see the problem. Contractually you're required to not mutate the array so ownership is effectively transferred to React anyway and it should be free to freeze it. Right?

[satya164]

createElement does create an array when you use the var args. We could probably freeze that one. Needs to be tracked first if it is common though.

If it's frozen, it'll show nothing in loose mode when trying to mutate, and I think it will be confusing.

[jimfb]

Even if it takes an array I don't see the problem. Contractually you're required to not mutate the array so ownership is effectively transferred to React anyway and it should be free to freeze it. Right?

Passing an object to React.createElement does not transfer ownership to React any more than passing the object to console.log transfers ownership to console. It would be confusing/surprising/bad if console.log decided to freeze whatever you passed in. Functions should generally avoid mutating their inputs. Allowing React.createElement to freeze the array is a form of mutation.

[syranide]

Passing an object to React.createElement does not transfer ownership to React any more than passing the object to console.log transfers ownership to console. It would be confusing/surprising/bad if console.log decided to freeze whatever you passed in. Functions should generally avoid mutating their inputs. Allowing React.createElement to freeze the array is a form of mutation.

@jimfb I have to admit I'm not entirely up on the internals here, if modifying children is a danger to React internals (which I assumed, but I guess I'm wrong now that I think about it) then it should reasonably be prevented from mutating.

If not, then we have previously said that children should be treated equally to other props. Such that children only have special treatment for convenience and passing the same data through other props should exhibit identical behavior. I'm not sure of your stance on the API internally nowadays, but previously, passing varargs or an array was considered synonymous. Freezing varargs would contradict that. For all intents and purposes, children does not need to be renderables and a component getting data through children rather than a prop can be a visual/convenience preference. So if we don't freeze props then freezing children is a bit odd, I agree it makes sense in the overwhelming majority of cases, but IMHO it does contradict "children being just another prop" if we still stand by it. tl;dr I feel like this boils down to whether we find it ok to consider vararg/JSX-children to be special or if they should be straight-up forwarded as an array. Also, what about nested children?

EDIT: PS. I'm not against making children special (essentially, they should only be used for renderables), I think it can make a lot of sense, but then perhaps we should be explicit about that too.

[jimfb]

@syranide Conceptually, I agree with you. There are edge cases like someone choosing a different name (children2) or mutating nested children. Ultimately this is a pragmatic attempt at catching the common cases of a particularly nasty antipattern. It's just a (really good) heuristic.

[sebmarkbage]

Define this in the non canDefineProperty case too.

[jimfb]

Done.

[ghost]

@jimfb updated the pull request.

[fab1an]

I wrote a HOC which changes the rendered element-tree: react-native-style-tachyons.

I needed a couple of hours to understand whether modifications of props, children etc. is allowed, how to copy keys and/or children, since there's not really much in-depth documentation for functions like cloneElement.

See my (unanswered) question on SO: https://stackoverflow.com/questions/37120956/react-cloneelement-pass-new-children-or-copy-props-children

Here my code that recursively changes an element-tree, I'm still not completely sure if I'm doing it correctly: https://github.com/tachyons-css/react-native-style-tachyons/blob/master/src/reactWrapper.js#L43

[sophiebits]

I answered the questions in your SO post.

[fab1an]

@spicyj Great thanks.