Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.0] Put params, location, and route on context.router #3325

Closed
ryanflorence opened this issue Apr 15, 2016 · 23 comments
Closed

[3.0] Put params, location, and route on context.router #3325

ryanflorence opened this issue Apr 15, 2016 · 23 comments
Labels
Milestone

Comments

@ryanflorence
Copy link
Member

ryanflorence commented Apr 15, 2016

These are all really useful deep in the tree, especially when building links. Instead of asking folks to take them from their route components and put them on context there, we should probably just do it.

Thoughts?

@taion
Copy link
Contributor

taion commented Apr 15, 2016

We want to be careful about what we mean by location – the location that the router sees is not the location that history sees, in the case of an async transition.

Otherwise it probably makes sense to put these on the context.

@timdorr
Copy link
Member

timdorr commented Apr 15, 2016

That location is the one after we have resolved the async stuff, correct? That would seem to be safer than history's, since it doesn't ever get out of sync with what the router is attempting to display. (re: the react-router-redux 4.0 changes).

@ryanflorence
Copy link
Member Author

ryanflorence commented Apr 15, 2016

"the location we used to render", whichever one RouterContext got from Router.

@taion
Copy link
Contributor

taion commented Apr 15, 2016

Makes sense to me. @timdorr, you'll want to add a caveat in React Router Redux that this won't match whatever location ends up in the Redux store (err, does React Router Redux still have an option to do that?).

@ryanflorence
Copy link
Member Author

ryanflorence commented Apr 16, 2016

Looking at the code, this will be a lot simpler after 3.0. Not that it's hard, it's just that where we provide context is pretty messy from all the deprecations. Maybe we should just ship this w/ 3.0 when we can clean all that stuff up.

May 9th is when we can release 3.0, so maybe let's figure out everything we want for the next minor release, release it, and then make a 3.0 branch to start removing deprecations and such (use a branch so that we can do some bug fixe releases from master if we need to).

@ryanflorence ryanflorence added this to the next-3.0.0 milestone Apr 16, 2016
@timdorr timdorr changed the title Put params, location, and route on context.router [3.0] Put params, location, and route on context.router Apr 19, 2016
@taion
Copy link
Contributor

taion commented May 8, 2016

What do we think of putting router on the props for route components as well, then?

@timdorr
Copy link
Member

timdorr commented May 9, 2016

Since we're doing that via withRouter, it makes sense to get it for "free" with a route component.

@gaearon
Copy link
Contributor

gaearon commented May 9, 2016

#3442

@timdorr timdorr closed this as completed May 9, 2016
@gaearon
Copy link
Contributor

gaearon commented May 9, 2016

We’ll be putting route later right? We can’t do it yet.

@taion
Copy link
Contributor

taion commented May 9, 2016

I think we defer that until we do relative links... they just come together really naturally.

@timdorr timdorr reopened this May 9, 2016
@timdorr
Copy link
Member

timdorr commented May 9, 2016

Whoops, forgot about 'route' (and Dre...)

@gaearon
Copy link
Contributor

gaearon commented May 9, 2016

We can track this in a separate issue since it’s implemented separately and after 3.0?

@taion
Copy link
Contributor

taion commented May 9, 2016

It'd be really great if we could get relative links in 3.0... just a question of time. Ideally they'd be part of core rather than an add-on, which would be a breaking change (but one that wouldn't break much).

@taion
Copy link
Contributor

taion commented May 12, 2016

Can we track the remaining bit on the "relative <Link to>" issue? From a code perspective, they work out very similarly.

@jwbay
Copy link
Contributor

jwbay commented May 18, 2016

What do we think of putting router on the props for route components as well, then?

Since we're doing that via withRouter, it makes sense to get it for "free" with a route component.

Any chance this a concrete goal for 3.0? Having to wrap my routed components with withRouter so they can do router.push feels awkward when there's other router props already injected by RouterContext.

@taion
Copy link
Contributor

taion commented May 18, 2016

This is all on next except for route

@jwbay
Copy link
Contributor

jwbay commented May 20, 2016

I don't think I see router on props. It would be here right? https://github.com/reactjs/react-router/blob/next/modules/RouterContext.js#L57-L63

I could take a stab at it if you'd take a PR.

@taion
Copy link
Contributor

taion commented May 20, 2016

Sounds reasonable to me, but you'll need one more person to approve the change.

@gaearon
Copy link
Contributor

gaearon commented May 20, 2016

We want to bring withRouter() to have parity with route handlers right? It would make sense to put .router there as well then.

@taion
Copy link
Contributor

taion commented May 20, 2016

My thoughts exactly. Sounds like that's the requisite 2 votes, then.

@timdorr
Copy link
Member

timdorr commented May 20, 2016

👍 from me so we have something "in-house"

@taion
Copy link
Contributor

taion commented May 20, 2016

Yeah the original justification is that we didn't want router in two places but that's irrelevant now that we have withRouter.

@taion
Copy link
Contributor

taion commented May 31, 2016

I'm closing this for now, given that the work to put route on context.router is closely tied to the relative link work.

@taion taion closed this as completed May 31, 2016
@remix-run remix-run deleted a comment Jul 12, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants