Add withRouter HoC #3352

timdorr · 2016-04-18T21:21:27Z

Closes #3350.

This is a little funky because we have to set a static on the composed component. I was originally just going to set Component.contextTypes and call it a day, but that creates a side effect. Is there any downside or error in the proposed solution? hoist-non-react-statics is nice to keep from running into unintended surprises (especially if the component is wrapped multiple times).

ryanflorence · 2016-04-18T21:36:51Z

Huh ... I was thinking

function withRouter(Component) {
  return React.createClass({
    contextTypes: { router: routerShape },
    render() {
      return <Component {...this.props} router={this.context.router}/>
    }
  })
}

That doesn't copy over the static methods, but ... I dunno ... does redux copy over the statics?

timdorr · 2016-04-18T21:39:41Z

Yes: https://github.com/reactjs/react-redux/blob/master/src/components/connect.js#L365

Also, that won't provide it on this.context.router, only this.props.router. Most of the docs assume this.context.router and may or may not include the contextTypes bit. I think in order to reduce confusion when people copypasta, we should keep it on this.context.router.

ryanflorence · 2016-04-18T21:41:43Z

Yeah, I want this.props.router and change all the docs.

Download · 2016-04-18T21:42:01Z

How about decoration over composition?

function withRouter(Component) {
  if (! Component.contextTypes) {Component.contextTypes = {};}
  Component.contextTypes.router = Router.PropTypes.router;
}

timdorr · 2016-04-18T21:42:40Z

@Download I didn't want to create a side effect.

taion · 2016-04-18T21:43:07Z

You want composition because the idea is to make consumers not care about context.

I'd definitely set displayName here so users can tell what's happening.

Download · 2016-04-18T21:43:40Z

@ryanflorence But why?
context is documented now. Why do you think it would be going away?

@timdorr What is the problem with a side effect? We are talking React components right? Mostly we just create them at app start and that's it. What issues do you see?

ryanflorence · 2016-04-18T21:45:07Z

@Download let's discuss justification on #3350, mind cut/pasting over to there your question?

Comments here should be about implementation.

timdorr · 2016-04-18T21:47:22Z

Yeah, I want this.props.router and change all the docs.

Ugh, I hear ya, that's just a lot more busywork.

Download · 2016-04-18T21:48:38Z

My reasoning is that, given React's user base, context is more dependable as an API than withRouter. Not to bash this project, but that's how I see it. Using context directly keeps my component a standard component whereas using props means that now I am almost forced to wrap it with withRouter. Instead of a 'dependency' on context I now have a 'dependency' on withRouter.

ryanflorence · 2016-04-18T21:50:09Z

Again, @Download let's not justify the feature here, just the implementation. #3350 is for justification, please move your questions over there.

ryanflorence · 2016-04-18T22:01:06Z

@timdorr #3350 (comment)

timdorr · 2016-04-18T22:10:11Z

Switched it up to this.props.router and stole some stuff from connect()

taion · 2016-04-18T22:14:21Z

modules/withRouter.js

+  WithRouter.displayName = `withRouter(${getDisplayName(WrappedComponent)})`
+  WithRouter.WrappedComponent = WrappedComponent
+
+  return hoistStatics(WithRouter, WrappedComponent)


Oh, wow, Redux does this? I feel like most wrappers these days don't. The Relay container doesn't, for example.

Yeah, it's just to avoid unexpected surprises from adding static methods. See reduxjs/react-redux#270 (comment)

Wow, @gaearon is unusually charitable to his users. 😛

Well, it’s React Router that used to expect willTransitionTo on statics 😄 .

Trailblazers gonna trail blaze.

taion · 2016-04-19T02:00:12Z

LGTM other than the test nit, BTW.

I assume we'll want to update docs on another PR, right? Otherwise it's going to get hairy.

timdorr · 2016-04-19T02:01:48Z

I was going to do the doc changes here too. All of that should go together.

ryanflorence · 2016-04-19T03:20:10Z

+1 to docs change on this commit, will be nice to see it together, sorry I know it's a pain, if you want me to do it, I can do it on my flight to Boston next week :)

timdorr · 2016-04-20T03:22:54Z

Added docs and updated the examples. Also forgot the export. Should be good to go.

taion · 2016-04-20T03:31:09Z

docs/API.md

@@ -162,6 +163,9 @@ Given a route like `<Route path="/users/:userId" />`:
 ### `<IndexLink>`
 An `<IndexLink>` is like a [`<Link>`](#link), except it is only active when the current route is exactly the linked route. It is equivalent to `<Link>` with the `onlyActiveOnIndex` prop set.

+### `withRouter(component)`
+A HoC (higher-order component) that wraps another component to provide `this.props.router`. Pass in your component and it will return the wrapped component.


Mention that it can also be used as a decorator on classes

Oops, that should have been a question – I mean, "do we want to mention that?"

I was thinking about that, but decorators aren't in babel 6 unless you include the non-default babel-plugin-transform-decorators-legacy. Given the state of flux surrounding them, I'm hesitant to bring attention to them right now.

taion · 2016-04-20T03:37:56Z

You don't need the explicit displayName in the examples – Babel will add it for you automatically, even with the wrapper. See e.g. http://babeljs.io/repl/#?evaluate=true&presets=es2015%2Creact&code=const%20Foo%20%3D%20withRouter(React.createClass(%7B%7D))%3B.

timdorr · 2016-04-20T03:40:09Z

I was getting unnamed components. Does that apply to Babel 5? (which we're still on!)

taion · 2016-04-20T03:51:39Z

I guess it must be a new Babel 6 feature. Bleh.

taion · 2016-04-20T04:02:16Z

I'm inclined to say we omit for the sake of terseness anyway.

taion · 2016-04-20T16:14:22Z

Maybe we should update to Babel 6... 👹

ryanflorence · 2016-04-20T17:30:06Z

Any reason we don't?

timdorr · 2016-04-20T17:38:06Z

I remember there being some issues with the examples in Babel 6. I think we figured all of that out, so I'm not sure why not. I can try out a PR later on to see how much work is involved and to centralize discussion.

taion · 2016-04-20T17:38:55Z

I just started on Babel 6. We need a custom transform to preserve module exports, and it's a bit of work to avoid breaking the ES build.

timdorr · 2016-04-20T17:53:57Z

Redux does that via an env: https://github.com/reactjs/redux/blob/master/.babelrc

taion · 2016-04-20T18:05:58Z

Yup, I know. Just need to tweak a bit to preserve some of our functionality.

taion · 2016-04-21T07:25:32Z

I see the automatic displayName logic working against master now. Want to check if it works for the cases in this PR?

Also add an export of withRouter

timdorr · 2016-04-21T07:28:48Z

Yep, I'll get rid of them now.

* Create a withRouter HoC. * withRouter passes this.props.router * Update docs, examples, and upgrade guide. Also add an export of withRouter * Doc tweaks. * Remove displayNames.

timdorr added this to the next-2.4.0 milestone Apr 18, 2016

timdorr added the feature label Apr 18, 2016

timdorr force-pushed the withRouter branch from 9f63d55 to 83d48e9 Compare April 18, 2016 21:43

taion reviewed Apr 18, 2016
View reviewed changes

timdorr force-pushed the withRouter branch 2 times, most recently from f24f046 to ced9448 Compare April 20, 2016 03:22

timdorr force-pushed the withRouter branch from ced9448 to e89a5cb Compare April 20, 2016 03:23

taion reviewed Apr 20, 2016
View reviewed changes

timdorr added 4 commits April 21, 2016 03:26

Create a withRouter HoC.

a7b0ffd

withRouter passes this.props.router

4c3f3e7

Update docs, examples, and upgrade guide.

d21de1e

Also add an export of withRouter

Doc tweaks.

344c86a

Remove displayNames.

61a3c37

timdorr force-pushed the withRouter branch from 0d27ee0 to 61a3c37 Compare April 21, 2016 07:30

taion merged commit 39a87cb into master Apr 21, 2016

taion deleted the withRouter branch April 21, 2016 16:12

grigio mentioned this pull request Apr 29, 2016

Support new Navigation API taofed/react-web#5

Closed

gaearon mentioned this pull request May 9, 2016

Make withRouter() update in response to context changes #3439

Closed

gaearon pushed a commit that referenced this pull request May 9, 2016

Add withRouter HoC (#3352)

3002ec3

* Create a withRouter HoC. * withRouter passes this.props.router * Update docs, examples, and upgrade guide. Also add an export of withRouter * Doc tweaks. * Remove displayNames.

renovate bot mentioned this pull request Oct 11, 2018

Update dependency react-router to v2.8.1 - autoclosed neolanders/min-starter-react-redux-axios#14

Closed

1 task

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Add withRouter HoC #3352

Add withRouter HoC #3352

Conversation

timdorr commented Apr 18, 2016

ryanflorence commented Apr 18, 2016

timdorr commented Apr 18, 2016 • edited Loading

ryanflorence commented Apr 18, 2016

Download commented Apr 18, 2016

timdorr commented Apr 18, 2016

taion commented Apr 18, 2016

Download commented Apr 18, 2016

ryanflorence commented Apr 18, 2016 • edited Loading

timdorr commented Apr 18, 2016

Download commented Apr 18, 2016 • edited Loading

ryanflorence commented Apr 18, 2016 • edited Loading

ryanflorence commented Apr 18, 2016

timdorr commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taion commented Apr 19, 2016

timdorr commented Apr 19, 2016

ryanflorence commented Apr 19, 2016

timdorr commented Apr 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taion commented Apr 20, 2016

timdorr commented Apr 20, 2016

taion commented Apr 20, 2016

taion commented Apr 20, 2016

taion commented Apr 20, 2016

ryanflorence commented Apr 20, 2016

timdorr commented Apr 20, 2016

taion commented Apr 20, 2016

timdorr commented Apr 20, 2016

taion commented Apr 20, 2016

taion commented Apr 21, 2016

timdorr commented Apr 21, 2016

timdorr commented Apr 18, 2016 •

edited

Loading

ryanflorence commented Apr 18, 2016 •

edited

Loading

Download commented Apr 18, 2016 •

edited

Loading

ryanflorence commented Apr 18, 2016 •

edited

Loading