Adding support to get the component instance that withRouter HOC wraps #3735
Conversation
| }, | ||
| render() { | ||
| const router = this.props.router || this.context.router | ||
| if(options && options.withRef) { |
There was a problem hiding this comment.
please match the code style elsewhere in the project
| render() { | ||
| const router = this.props.router || this.context.router | ||
| return <WrappedComponent {...this.props} router={router} /> | ||
| if(options && options.withRef) { |
|
Just to address @taion's comments at a higher level, we'll be switching to a callback ref in the next version of react-redux. You can see that we set an instance variable instead of having a function call, and use a callback ref for setting it. That jives better with how React wants to handle refs in the future (i.e., removing string refs) and makes it a little easier to work with when you want to access that ref. |
|
|
||
| function getDisplayName(WrappedComponent) { | ||
| return WrappedComponent.displayName || WrappedComponent.name || 'Component' | ||
| } | ||
|
|
||
| export default function withRouter(WrappedComponent) { | ||
| export default function withRouter(WrappedComponent, options) { |
There was a problem hiding this comment.
I would instead use destructuring here. It essentially makes the code self-documenting. So, { withRef } instead of options.
There was a problem hiding this comment.
That'd require doing e.g. { withRef: false } = {}, though. The transpiled code for default params is a bit ugly, plus there's that extra temporary object.
There was a problem hiding this comment.
Huh? You can just do withRouter(WrappedComponent, { withRef = false }). But the default is undefined, so it's falsey and would work all the same.
There was a problem hiding this comment.
That doesn't work unless the caller explicitly passes in an object as the 2nd arg: http://babeljs.io/repl/#?evaluate=true&lineWrap=false&presets=es2015-loose&experimental=true&loose=true&spec=false&code=function%20foo(%7B%20withRef%20%7D)%20%7B%0A%20%20console.log(withRef)%3B%0A%7D%0A%0Afoo()%3B&playground=false
There was a problem hiding this comment.
Ah, shit, you're right. Forgot about that.
|
LGTM. There's one small style change I want to make, but no need to go back and forth yet again on that. I'll clean it up after the fact. |
#3735) * More code style fixes * Fixing code style, and updating ref to be a function ref * updating error message from not passing in proper options object * Updating withRouter based on comments * Reverting change to withRouter * Last minute spacing issues * Fixing some spacing issues * withRouter now supports withRef as an option, to better support getting the wrapped instance
|
Oh, weird, how did this get double-merged? @timdorr |
|
I squashed after the fact. |
Following up on conversation from this bug. Adding the ability to specify an withRef option to be passed to withRouter. The signature has changed from
withRouter(MyComponent)towithRouter()(MyComponent)except now you can do thiswithRouter({withRef:true})(MyComponent)and laterinstanceOfMyComponent.getWrappedInstance()returning the component instance wrapped by withRouter. This feature is also demonstrated in react-redux in code and is documented. If given the thumbs up I will update the documentation, and write a codemod script.