1.1 context.router #2647
1.1 context.router #2647
Conversation
the upgrading information is jumping ahead a bit, while I’d love the upgrading information to match perfectly with then commits, I think the best way to communicate a clear upgrade path is to do it cohesively, so I’ll be editing this further when the other commits land, just dumping it in now so we don’t forget it!
|
@timdorr ^ see above if you've got some time to help out |
| return history.isActive(...args) | ||
| } | ||
| } | ||
| return deprecateObjectProperties({ history, location, router }, { |
There was a problem hiding this comment.
Maybe __DEV__-gate this? Just to strip the warning messages and the extra level of indirection out of production bundles.
EDIT: Should have read the full thing first. Sorry.
There was a problem hiding this comment.
maybe there's a cleaner API for deprecateObjectProperties, it would be nice to get these large strings out of production builds.
There was a problem hiding this comment.
For warning and invariant, it's a Babel plugin that strips out the warning messages in production builds.
But I feel like any effort we might want to put into that sort of thing would be better spent on providing codemods for our users to shift to the new API, rather than deprecation ergonomics on our end.
There was a problem hiding this comment.
updated, messages no longer get included in a dev build
| class RoutingContext extends Component { | ||
|
|
||
| getChildContext() { | ||
| const { history, location } = this.props | ||
| return { history, location } | ||
| const router = { |
There was a problem hiding this comment.
Micro-optimization-ville, population me: it might be good to put this in componentWillReceiveProps after a reference equality check, to avoid re-building the router object unnecessarily.
There was a problem hiding this comment.
first tell me how long it takes to do 1 million times in a row.
There was a problem hiding this comment.
Probably 5% to 10% as long as it takes for the GC to clean up all of the replaced router objects :p
But yeah it's probably a pointless micro-optimization.
|
@ryanflorence Is it your intention to do this in multiple PRs? Or all at once? I changed the title but then realized that maybe you'd like to merge this right now before we merge other stuff yet to come. |
|
We can merge now. I see you're also still on GMT this morning. |
|
Haha, yes. No sleep for me tonight :/ K, I'll go ahead and merge. |
| return deprecateObjectProperties(contextExport, { | ||
| history: '`context.history` is deprecated, please use context.router', | ||
| location: '`context.location` is deprecated, please use a route component\'s `props.location` instead. If this is a deeply nested component, please refer to the strategies described in https://github.com/rackt/react-router/blob/v1.1.0/CHANGES.md#v110' | ||
| }) |
There was a problem hiding this comment.
Unfortunately, because of the way this is implemented, you'll run into these deprecation warnings all the time in development. When React goes to check contextTypes, it will run into the getter and fire off the warning. Doesn't matter if you're using the API correctly, it always occurs.
Peep the commit message.
/docs. Don't worry about CHANGES.md. The changes coming down the pipe after this will require a more cohesive explanation of how to update your code than we can do with each individual commit that changes API.Linkand in our tests), but that should maybe be a separate pull request just to "prove" that the old API still works (afaict it does, tests pass anyway!).