infinitered / reactotron

A desktop app for inspecting your React JS and React Native projects. macOS, Linux, and Windows.
https://docs.infinite.red/reactotron/
MIT License
14.89k stars 945 forks source link

Support ref passing on overlay component #625

Closed khagesh closed 5 years ago

khagesh commented 6 years ago

I am creating app screens with react navigation. I get AppNavigator which holds screen and router logic. AppNavigator is the top level UI component. I am adding overlay to it. This is how I am doing it.

if (__DEV__) {
  // $FlowFixMe Tried to extend console interface, but it didn't work
  AppNavigator = console.tron.overlay(AppNavigator)
}

Now I use AppNavigator in app.js as

render(){
  return <AppNavigator ref={navigator => this.navigator = navigator} />
}

I don't get instance in ref and gets an error that Stateless functional components cannot be given refs..

Information

Proposed solution

I managed to track down the issue here https://github.com/infinitered/reactotron/blob/b799e69de4f19312d29adfe139b0d220c41a0a6f/packages/reactotron-react-native/src/plugins/overlay.js#L25 We are returning a functional component from Reactotron.overlay and that's why we can't add refs. Can we please change this to a class based component. I can raise a PR if you are okay with this change. Also, is there any specific reason on why we can't return class based component from Reactotron.overlay

skellock commented 6 years ago

We could, but our ref would be pointing to the wrong thing. I’ve seen people create a innerRef prop that does this (e.g. StyledComponents). Then you’d have one path with an innerRef and another without, which is less than ideal.

How about using the overlay hoc up in app.js instead?

const Nav = () => <AppNavigator ref=... />
return __DEV__ ? console.tron.overlay(Nav) : Nav 
khagesh commented 6 years ago

I can certainly add overlay to the top most component.

We could, but our ref would be pointing to the wrong thing.

I don't understand why your ref will be pointing to wrong thing. From this line https://github.com/infinitered/reactotron/blob/b799e69de4f19312d29adfe139b0d220c41a0a6f/packages/reactotron-react-native/src/plugins/overlay.js#L27 We are passing props to WrappedComponent so application developers ref will be passed to WrappedComponent and reactotron ref should stay intact.

You can go ahead and close the issue because I can add it to top most level component and get the same functionality. I opened the issue because I thought if we can change it may be we can support inner level components overlay as well.

skellock commented 6 years ago

Perhaps I am misunderstanding how the ref thing works. I likely am.

If you know this would work, I’m up for what you are suggesting!

rmevans9 commented 5 years ago

Closing this issue.