Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

fix resolving ReactComponentElement children #202

Merged
merged 2 commits into from
Jun 12, 2015

Conversation

azazdeaz
Copy link
Contributor

@azazdeaz azazdeaz commented Jun 9, 2015

Currently Radium resolves all the childrens of the enhanced element which can lead to some unexpected behavior and affect the performance badly.
ex:

<MyRadiumComponent>
  <ThirdPartyComponent/>
  <div/>
</MyRadiumComponent>

The ThirdPartyComponent and it's children will be resolved, not just the div

<MyRadiumComponent>
  <MyRadiumComponent>
    <MyRadiumComponent/>
  </MyRadiumComponent>
</MyRadiumComponent>

The inner MyRadiumComponent-s will be added to the resolveStyles() multiple times.

I added a failing test for this and a possible fix which is only resolve ReactDOMElement-s when it recurse over the children in the resolveStyles() and skips the `ReactComponentElement``-s.

@@ -137,6 +137,14 @@ var resolveStyles = function (
// will not be here, so each component needs to use Radium.
var newChildren = null;
var oldChildren = renderedElement.props.children;
var resolveChild = function (child) {
// test if child is a valid ReactDOMElement and not ReactComponentElement
if (React.isValidElement(child) && typeof child.type === 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the extra closure, which I find a bit hard to follow, let's just invert this check and return early from resolveStyles directly after the children resolution:

if (!React.isValidElement(renderedElement) || typeof renderedElement.type !== 'string') {
  return renderedElement;
}

The check must come after child resolution because you could pass dom elements as children of custom components, and you'd want them to get resolved correctly. Please add a test like the following:

var renderedElement = (
  <div>
    <CustomComponent style={style}>
      <div style={style} />
    </CustomComponent>
  </div>
);

Also, once you add the check, you can remove it from inside the callback in React.Children.map.

@azazdeaz
Copy link
Contributor Author

@ianobermiller You're right! I added the changes, what do you think?

}

return React.cloneElement(
renderedElement, renderedElement.props, newChildren
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it doesn't fit on one line in 80 chars or so, drop each param to its own line

@ianobermiller
Copy link
Contributor

We can clean that up another time, I'll probably make a refactoring pass at resolveStyles in the future.

ianobermiller added a commit that referenced this pull request Jun 12, 2015
fix resolving ReactComponentElement children
@ianobermiller ianobermiller merged commit 13a484f into FormidableLabs:master Jun 12, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants