-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
ReactDOMTextComponent needs getInstance() #2654
Comments
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?
|
I did bring in your other diffs. These aren't new failures but were probably just missed or hidden before. I believe |
@sebmarkbage What should |
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.
|
As per subsequent in-person conversation with @sebmarkbage, we've decided to update the scry instead of adding a getPublicInstance() method to TextComponent |
Fixed in #2695 |
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
The text was updated successfully, but these errors were encountered: