Skip to content

Conversation

sophiebits
Copy link
Collaborator

Fixes #2770.

Included: Refactor empty component handling; now doesn't use ReactCompositeComponent and ._currentElement is actually null/false.

Now doesn't use ReactCompositeComponent and `._currentElement` is actually null/false.
@sophiebits
Copy link
Collaborator Author

Opinions on the circular dependency here for instantiateReactComponentReactEmptyComponentinstantiateReactComponent? This has instantiateReactComponent pass a reference to itself when instantiating ReactEmptyComponent. (We have this problem in two other places already: in ReactDOMComponent we get around the dependency issue by injecting the cycle via ReactNativeComponent; in ReactCompositeComponent we export an incomplete class and instantiateReactComponent completes it with a reference to itself.)

@sophiebits sophiebits added this to the 0.14 milestone Sep 10, 2015
@sebmarkbage
Copy link
Collaborator

Do we need to update anything in devtools?

@sebmarkbage
Copy link
Collaborator

WRT the cycles, I think we'll end up merging all cyclic code into ReactReconciler eventually but this is fine for now.

@sophiebits
Copy link
Collaborator Author

Well this is what happened before in the devtools, apparently:

image

But yes. facebook/react-devtools#199 does that.

@sebmarkbage
Copy link
Collaborator

Ok. Lgtm. Let's try it.

Side note: I always thought we allowed true to be empty as well. Isn't that what happens for traverseAllChildren?

@sophiebits
Copy link
Collaborator Author

Yes, that is what happens for real children. We also allow undefined in that case, and empty string makes an empty text node which really could be nothing as well. I was thinking on my way home that maybe we should rescind our support for false and support only null… but maybe supporting everything is simpler, easier, and more consistent.

sophiebits added a commit that referenced this pull request Sep 10, 2015
Preserve DOM node when updating empty component
@sophiebits sophiebits merged commit dc2570e into facebook:master Sep 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants