ReactDOMTextComponent needs getInstance()

[zpao]

With the recent separation of internal and external instances ReactDOMTextComponents got left out. Certain tools (namely ReactTestUtils) expect that everything in the rendered tree conforms to a certain API. Attempting to sync the last couple weeks of React to FB is causing tests which depend on ReactTestUtils.findAllInRenderedTree to fail as a result.

cc @spicyj @sebmarkbage

[sebmarkbage]

Unit tests should not get the instance off text components because there shouldn't be an instance and consecutive text components might be merged as an implementation detail.

I thought I cleaned up all internal unit tests that relied on this in my diff? Did you patch in the fixes? Are there new ones?

Or is there something in findAllInRenderedTree that always breaks?

On Dec 3, 2014, at 8:22 PM, Paul O’Shannessy notifications@github.com wrote:

With the recent separation of internal and external instances ReactDOMTextComponents got left out. Certain tools (namely ReactTestUtils) expect that everything in the rendered tree conforms to a certain API. Attempting to sync the last couple weeks of React to FB is causing tests which depend on ReactTestUtils.findAllInRenderedTree to fail as a result.

cc @spicyj @sebmarkbage

—
Reply to this email directly or view it on GitHub.

[zpao]

I did bring in your other diffs. These aren't new failures but were probably just missed or hidden before.

I believe findAllInRenderedTree will always break if there's a text component since we blindly just call getPublicInstance() https://github.com/facebook/react/blob/master/src/test/ReactTestUtils.js#L127-L132

[sophiebits]

@sebmarkbage What should React.render("foo", el) return? (That doesn't work at all yet.)

[sebmarkbage]

Probably null

I was explicitly leaving it to throw for now to validate the assumption that nothing should be trying to get the public instance on text components.

Looks like we need to fix the scry. Not sure why that didn't show up in my tests.

On Dec 3, 2014, at 11:35 PM, Ben Alpert notifications@github.com wrote:

@sebmarkbage What should React.render("foo", el) return? (That doesn't work at all yet.)

—
Reply to this email directly or view it on GitHub.

[jimfb]

As per subsequent in-person conversation with @sebmarkbage, we've decided to update the scry instead of adding a getPublicInstance() method to TextComponent

[jimfb]

Fixed in #2695