use hoist-non-react-methods instead of hoist-non-react-statics

[elado]

copy all wrapped component's methods (prototype and static except react methods) into the wrapper component.

so methods like focus on @connect() class Composer { focus() { this.refs.input.focus } } will still be available if Composer is refed somewhere else.

Original issue: #267

hoist-non-react-methods is here: https://github.com/elado/hoist-non-react-methods

[gaearon]

This seems fragile and burdening to support to me, even via an external library.

I'd say at this point you're better off using getWrappedInstance(), or better, accept a prop like inputRef in your component.

class MyComponent extends Component {
  render() {
    return (
      <div>
        <input ref={this.props.inputRef} />
      </div>
    );
  }
}

class ParentComponent extends Component {
  componentDidMount() {
    this.input.focus();
  }

  render() {
    return (
      <MyComponent inputRef={input => this.input = input} />
    )
  }
}

With this pattern you can also “propagate” refs just by accepting it as a prop in parent component as well:

class ParentComponent extends Component {
  render() {
    return (
      <MyComponent inputRef={this.props.inputRef} />
    )
  }
}

Then you can perform the focus from a grandparent or further without accessing the instances.

[elado]

@gaearon:
Currently, static methods are hoisted, why not prototype methods? What's the difference? Current implementation just seems incomplete.
With this functionality, @connect can be added or removed without caring about changing how refs are managed from containers.
I integrated this into my app and it works flawlessly.

A. Can you think of a scenario where it wouldn't be desired?
B. If I change the code to hoist methods optionally, e.g. { withRef: true, hoistMethods: /* whitelist */ ['focus', 'scrollToBottom'] } or { withRef: true, hoistMethods: /* all methods */ true }
would this be merged?

[elado]

Also:

composer.focus doesn't necessarily mean input.focus(). It could be opening a menu. I wouldn't want to expose internal component types.

messageList.scrollToBottom() interacts with some div's scrollTop and I defintely wouldn't want to repeat that code over and over in containers of a <MessageList/> if all I expose is the div.

[gaearon]

Currently, static methods are hoisted, why not prototype methods? What's the difference? Current implementation just seems incomplete.

Static methods are hoisted because that's where people tend to put data fetching methods. There's no workaround here, and call sites are often generic doing something like "grab every fetchData static on components matched by router". So it's worth it there.

In case of methods on the prototype, it's fragile (can't name methods with same names as connect()ed class methods). This means if I later rename a method inside connect() I can break somebody's code. I don't want my implementation details to leak into consuming projects like this. Also, component instance interface is props. For static methods, we're still "out of React land" but with instances, you have props, and you should consider them component API.

With this functionality, @connect can be added or removed without caring about changing how refs are managed from containers.

This is a non-goal. If you're accessing instances directly you need to be aware that there is an intermediate instance. We don't want to pretend there isn't because that will just be more confusing and magic and will force us to deal with even worse edge cases (like the naming problems I suggested above).

I integrated this into my app and it works flawlessly.

I don't doubt that it works. I just don't want to support it in the library in terms of docs, bugs, feature requests (“a method works but a getter doesn’t!”, “inherited methods don’t show up!”, “third party mixin has a handleChange method and it clashes, I can’t change the mixin!”), etc. You can write your own connect() that uses connect() from React Redux but adds this functionality.

Can you think of a scenario where it wouldn't be desired?

I hope examples above show why I don't want to support this.

B. If I change the code to hoist methods optionally

No, that's more configuration to support and doesn't help with the bug / feature request burden and potential name clashes.

composer.focus doesn't necessarily mean input.focus(). It could be opening a menu. I wouldn't want to expose internal component types.

Use props when possible. What's wrong with passing <Composer isOpen> and doing the side effect in componentDidUpdate? Apart from one-off things like focus() and scrollIntoView() I think it's best to do this via props.

And you don't have to pass nodes via callback props either. You can do something like

class MyComponent extends Component {
  componentDidMount() {
    this.props.provideController({
      open: () => ...,
      close: () => ...,
      focus: () => ...
    });
  }

  componentWillUnmount() {
    this.props.provideController(null);
  }

  render() {
    return (
      <div />
    );
  }
}

class ParentComponent extends Component {
  componentDidMount() {
    this.ctl.open();
  }

  render() {
    return (
      <MyComponent provideController={ctl => this.ctl = ctl} />
    )
  }
}

[elado]

All your concerns about future bugs/naming conflicts are valid.

Apart from one-off things like focus() and scrollIntoView() I think it's best to do this via props.

These are exactly the methods that I would expose. A prop isFocused={shouldFocus} doesn't make sense -- it will need to turn its value into false after executing focus() - that's what methods are for. HTMLInputElement has a prop for value and a method for focus().

isOpen seems fine to be in a prop, although loading componentDidUpdate with comparing prev and new props seems ugly (yes, a matter of preference).

A prop like provideController like your example can be the solution. I don't care much about the boilerplate, but only about changing expected React's API, especially when exposing a component in a module.

[gaearon]

I think your second best bet is

You can write your own connect() that uses connect() from React Redux but adds this functionality.

if this is a common pattern in your codebase.

Cheers!

[elado]

👍 thanks.

[eduedix]

regarding #270 (comment) , how can I access/refer to the <input> element within MyComponent ?

[eduedix]

I changed my mind and used getWrappedInstance() instead. Thank you anyway.

[bchenSyd]

Dan @gaearon , thanks for your great input!

I'm using your suggestions to implement my wizard. Basically what i'm doing is when clicking the next button in Parent component (the wizard container), I need to call the submit function of Child component (the wizard page).

before I wrap my child component with connect, I was able to call wizard page's submit method by wiring up Parent -> Child thru ref, i.e

class parent extends Component{
 onNextButtonClick(){
  this.activePage.submit()
}
  render(){
    return <child ref={ref=>this.activePage = ref}  />
 }
}

and it worked fine.

I then found out that I need to wrap my child component to a connect component , and after that, I lost control to the Child component, as the activePage is now pointing to the wrapper component, which doesn't have submit method defined (only static function hoisted, not prototype)

Yes, I can use getWrappedInstance() which I believe will work just fine, but I really like your idea of passing callbacks to child and let child set the value that Parent is interested in, so I decided to give it a go

class Parent extends Component{
 onNextButtonClick(){
    this.activePage.submit()
}
  render(){
    return <child setRef={ref=>this.activePage = ref}  />
 }
}

class Child extends Component{

  componentWillMount() {       
       this.props.ownProps.setRef(this)
   }
   componentWillUnmount() {
       this.props.ownProps.setRef(null)
   } 
  submit(){
   //submit the form
  }

  render(){
    return <div> <form>whatever</form</div>
 }
}


const mapStateToProps = (state, ownProps)=>{
    return{
        activationStore : state.get(constants.NAME),
        ownProps
    }
}
const mapDispatchToProps = dispatch=>{
    return {
        actions: bindActionCreators(actionCreators,dispatch)
    }
}

export default connect(mapStateToProps, mapDispatchToProps)(BusinessInfo)

and this also works fine as well. great!

my question, is that when passing a callback to child, why can't we name the prop 'ref', i.e.

inside parent render
 render(){
    return <child ref={ref=>this.activePage = ref}  />
 }

won't work and will cause exception at child component, saying this.props.ownProps.ref is undefined, but I have defined it and passed down to child.. don't really understand why I got that error

does this mean that connect component will selectively filter out some props?

[gaearon]

This is because ref attribute, just like key, has special meaning in React, and is not a prop. We might support ref forwarding in React someday: facebook/react#4213.

[bchenSyd]

Great. Thanks Dan again!