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

Hardcode decorated component ref #35

Closed
wants to merge 1 commit into from

Conversation

jhollingworth
Copy link
Contributor

Not setting the ref of the inner component makes it a pain to test through refs.

Is there any specific reason for setting the ref through setUnderlyingRef or could we just set the ref to a string?

@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

Is calling getUnderlyingRef() a burden in this case?

@jhollingworth
Copy link
Contributor Author

Unfortunately yes. We're using react-test-tree which wraps a component and any children in a testable interface (Essentially a page object). Not having a ref present in the component means it cannot find any children.

Also, the fact that you have to call getUnderlyingRef() at all is a bit of a smell. The higher order component is really just an implementation detail and you rarely want to test it directly, rather the impact it has on the inner component. You can tell react-test-tree to make HoC's invisible (Example) which makes testing much more pleasant. Again, not having a ref there makes this impossible.

Out of all the issues I've just raised this is the only one that is completely blocking us from moving to redux 1.0. We used to use the <Connector .../> which allowed us to support our desired syntax but thats now gone...

What's your motivation behind not making the decorated component available via element.refs.underlyingRef considering the existing getUnderlyingRef() API is still supported?

@jhollingworth jhollingworth changed the title Hardcode decorated component to Hardcode decorated component ref Aug 9, 2015
@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

Out of all the issues I've just raised this is the only one that is completely blocking us from moving to redux 1.0.

Wait wait. Redux is in no way related to React Redux. You can use redux@1.0.0-rc with react-redux@0.2.1 (or whatever worked for you) just fine while we're figuring it out..

@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

Also, the fact that you have to call getUnderlyingRef() at all is a bit of a smell. The higher order component is really just an implementation detail and you rarely want to test it directly, rather the impact it has on the inner component.

Why not export the vanilla component then and test it directly without the HOC? I'm just trying to understand the issue better.

Are you testing the whole tree instead of particular components?

@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

I'm OK with merging this, but let's rename getUnderlyingRef() to getInstance() and refs.underlyingRef to refs.instance?

@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

I changed names a bit and put it in b7ef178, will release soon. Thanks for the tip.

@gaearon gaearon closed this Aug 9, 2015
@jhollingworth
Copy link
Contributor Author

Awesome thanks 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants