Skip to content
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

Return component._instance if getPublicInstance returns null #359

Merged
merged 1 commit into from
May 8, 2016

Conversation

aweary
Copy link
Collaborator

@aweary aweary commented May 2, 2016

Resolves #356

getPublicInstance returns null for SFCs and the _renderedComponent appears to be a wrapper component that doesn't actually have a render method. This causes findDOMNode to fail as it uses the render method to validate that an object is a React component.

This returns the internal _instance instead of returning component when inst === null.

I was thinking of maybe doing return component._instance || component to be extra safe but it didn't seem to make a difference.

@aweary
Copy link
Collaborator Author

aweary commented May 3, 2016

ping @ljharb while I know you're around 😄

@@ -54,7 +54,7 @@ export default function createWrapperComponent(node, options = {}) {
const component = this._reactInternalInstance._renderedComponent;
const inst = component.getPublicInstance();
if (inst === null) {
return component;
return component._instance;
Copy link
Member

Choose a reason for hiding this comment

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

at this point we're not really calling getPublicInstance at all. These lines could change to return component._instance perhaps (relevant commits: d395726, eg)

I'm wary of changing what appears to be the sole purpose of this method (to have different logic for SFCs). Could we change where findDOMnode is called instead? I'm not sure what would be best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of the methods that call findDOMNode are wrapped in single calls which pass in this.node which is returned by getWrappedComponent so this seems like the most logical place to make the change.

Unless there's another method to get the instance for SFCs specifically none of the methods dependant on findDOMNode will work (html, text, simulate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking about it more, it seems like React doesn't want you to be able to call findDOMNode on SFCs so the alternative might just be documenting that those methods don't work on SFCs.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively/additionally, throwing a more helpful error here

@ljharb
Copy link
Member

ljharb commented May 6, 2016

@aweary would you mind rebasing and force pushing this, so that there's no merge commits?

@aweary aweary force-pushed the sfc-instance branch 3 times, most recently from 766cb61 to 2768a9a Compare May 6, 2016 19:19
@aweary
Copy link
Collaborator Author

aweary commented May 6, 2016

@ljharb done and done 👍

@lelandrichardson lelandrichardson merged commit b106257 into enzymejs:master May 8, 2016
mathieuancelin pushed a commit to mathieuancelin/enzyme that referenced this pull request May 8, 2016
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