remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
52.79k stars 10.23k forks source link

Relative <Link to> #2172

Closed Robinfr closed 8 years ago

Robinfr commented 8 years ago

Right now it seems that the Link component is always absolute. It would be nice that if the to URL does not start with a / that it becomes relative to the current route.

E.g. current route is: http://localhost/#/zoo/

<Link to="giraffe">Giraffe</Link>

Clicking on the link should bring me to http://localhost/#/zoo/giraffe/ instead of http://localhost/#/giraffe/

mjackson commented 8 years ago

Yes, I agree. This is a feature I'd love to support.

knowbody commented 8 years ago

I do like it, however I wouldn't make it a default Link behaviour.

Sometimes I just like that "I don't need to think" what path my component is at and I just use the absolute path (as it is now). But I do agree that it would be useful to support relative path.

mjackson commented 8 years ago

Ideally it would work just like <Route path>, IMO. If it's relative, the link is relative. If it's absolute, the link is absolute.

knowbody commented 8 years ago

oh yeah! I might work on that tomorrow

jussi-kalliokoski commented 8 years ago

:+1: this would make routes easier to compose too without having to know where they're placed, e.g. order form with links to the different stages of it can be reused in multiple contexts:

can just be declared as (component: omitted for brevity):

// routes.js

const NewOrderForm = {
    path: "orders/new",
    childRoutes: [{
        path: "contact-details",
    }, {
        path: "billing-information",
    }, {
        path: "confirmation",
    }],
}

const DashBoard = {
    path: "dashboard",
    childRoutes: [
        NewOrderForm,
    ],
}

export default [{
    path: "/",
    childRoutes: [
        NewOrderForm,
        Dashboard,
    ],
}]
jussi-kalliokoski commented 8 years ago

FWIW, relative paths would also be handy on pushState/replaceState for symmetry (I'm using redux and some of my actions have redirects in them, so coming back to this order form thing, ORDER_CREATED redirects to a success page that's relative to the order form).

mjackson commented 8 years ago

relative paths would also be handy on pushState/replaceState for symmetry

I don't think I agree here. pushState and replaceState are an imperative API and can be called from practically anywhere. But <Link to> is a declarative API that should always render at the same URL.

jussi-kalliokoski commented 8 years ago

But is a declarative API that should always render at the same URL.

Ahh so would this actually make the links relative to route that they're declared in instead of the current route?

If not, while pushState and replaceState can be called from practically anywhere, that's still within a context (a browser window can only have one URL at a time), so I'm not sure I see the problem. It would be far more useful than the current behavior which treats relative URLs as absolute (which is worse than throwing an error, IMO).

qimingweng commented 8 years ago

In addition to relative Link redirects, something I do often is this:

/* replaceState would be history.replaceState or something similar, this just wraps it */
function addQuery(replaceState, path, query = {}, next = {}) {
  const nextQuery = assign({}, query, next);
  replaceState(null, path, nextQuery);
};

And I think the addition of new query keys could also be made easier. I originally mentioned this in #2226 but not sure if this issue might also cover it.

I agree that it's also useful to be able to wipe it clean, but I think it creates harder to maintain apps when I have to pass around location strings that are irrelevant to the rest of the logic but are required by replaceState

taion commented 8 years ago

Wouldn't you do something like

replaceState({query: {...query, ...next}, ...location})

?

qimingweng commented 8 years ago

@taion yes that looks nicer, but the caller still needs to know about the current query and location - I think maybe that shouldn't be necessary in all cases.

Not that I want to generalize too many components, but just that those components don't necessarily need to know their location for any other reason, so I find it extraneous.

taion commented 8 years ago

this.context.location is a thing now if you weren't already aware.

qimingweng commented 8 years ago

@taion yup, that's how I get the location right now, but it just seems like this is a little extraneous to the core utility of the component; does replaceState know it's current state?

taion commented 8 years ago

If you don't specify things in full, how do we know whether you want to merge or replace the query?

qimingweng commented 8 years ago

@taion You guys obviously have a vision for the API, but perhaps there would be a separate replaceQuery function for just that purpose.

However, it is actually pretty common for the query object to already exist, since usually that is something which determines what to render.

My initial instinct is:

replaceState(
  state: Object?
  location: String? // if null, then use current location
  query: Object? // if null, then use current query (this would replace the whole query if it exists)
)

and

replaceQuery(
  mergeObject: Object // object to merge into current query
)

Obviously adding a replaceQuery function is a huge, but just throwing it out there in case it catches on.

taion commented 8 years ago

I don't have a vision, I just want a good/usable/clear API that I can build my products on top of. I think something like a replaceState API that takes something like a location object is a cleaner API than replaceQuery, and I'd rather use that and just incur the cost of pulling location off context if I want to do something relative - but that's just me.

qimingweng commented 8 years ago

@taion agreed that it's a little messier, but the boilerplate is gettng a little overwhelming. I wonder if there are other alternatives, perhaps higher order functions to wrap replace state or a HOC that injects replaceState... Actually that might not be too bad.

taion commented 8 years ago

Certainly if you need a special use case, that might make sense. I don't think what I have above looks too bad with object spreads, though, and it's nice in being very explicit, which I think is a good property for a general-purpose API.

jussi-kalliokoski commented 8 years ago

Ahh, now I actually see the problem with relative URLs in pushState and replaceState:

a browser window can only have one URL at a time

So for example doing something async and pushState'ing to a new URL after that - the user (or something else) might have navigated to another URL already, where the relativity of the URL no longer holds, leading to a broken redirect. Of course, force redirecting when the user already navigated away is problematic otherwise as well, but that's a separate topic and a lesser evil than redirecting the user to a 404.

taion commented 8 years ago

I'm a little confused by the idea of relative <Link to> in the context of nested routes. Would the links be relative to the current location, or just the portion of the location matched by the relevant route corresponding to the route component with the <Link>?

It should have to be the former, I think, but then that means that you wouldn't be able to reliably use relative links except on leaf route components, which is a bit weird.

taion commented 8 years ago

My confusion is this. Suppose your routes look like:

<Route path="/foo" component={Parent}>
  <Route path="bar" component={Child} />
</Route>

Suppose you are on the path /foo/bar. <Parent> renders a <Link to="baz">.

Where does this link take you? /baz? /foo/baz?

What if instead you're on the path /foo/bar/? Currently the router treats this as being the same as foo/bar, but it's not clear to me that the answer should be the same as above.

I think the real challenge here is specifying what the correct behavior should be.

knowbody commented 8 years ago

The problem of relative URLs has been solved long time ago. Maybe I'm wrong but I think that we are just overthinking the problem here. And instead of thinking "what if" we should just follow the already specified standards. Have a look at RFC1808

taion commented 8 years ago

Two problems:

Robinfr commented 8 years ago

OK guys, since this is generating so many comments I'll post my idea on how it should work as well.

It's quite simple. The use cases are exactly the same as with a normal relative URL, you wouldn't use it in places where you are not sure of the base URL (e.g. menus).

You only use a relative Link in places where you can be absolutely sure that the base URL is always the same or that the structure is always the same, e.g.:

When I am on the http://localhost/#foobars url I have a list of foobars. Each item has its own foobar page because I have set up a route that looks like: foobars/:foobarId. In this case it would be very nice to use a relative URL, e.g.: Foobar 2.

Again, you wouldn't use this when you are not certain of the structure of the URL.

taion commented 8 years ago

I feel like that proposal is undesirable for a few different reasons.

  1. One of the core features of React Router is nested routing; saying that a feature doesn't work with nested routes just feels wrong. Furthermore, this would be confusing and opaque to users - relative links would break as soon as a user added child routes to an old routes, and only allowing relative links in specific places just seems frustrating and error-prone.
  2. This is still underspecified - as I mentioned above, the router treats /foo and /foo/ the same, but "default" handling of relative links would treat the two different.
  3. It's not possible to assert correctness, i.e. that relative links aren't used from e.g. menus, because we can't match a <Link> right now to the route component that it belongs to.

I haven't worked out all the details yet, but I think relative links should work like this, from both a spec and a technical PoV:

  1. Relative links replace the path segment of the component to which they associated. If your routes are /foo > bar, and you are on /foo/bar, a link to baz on the parent will take you to /baz, while a link to baz on the child will take you to /foo/baz
  2. To effect this, we need to wrap each route component in a context provider informs nested <Link>s of which component they belong to

(2) means this might have to be opt-in, though.

agundermann commented 8 years ago

Agree with @taion . I don't think it makes much sense to use the current full path when a relative pathname is given. For queries and hashes it would be useful though.

Relative links replace the path segment of the component to which they associated. If your routes are /foo > bar, and you are on /foo/bar, a link to baz on the parent will take you to /baz, while a link to baz on the child will take you to /foo/baz

Another idea could be to start matching from the end of the current segment, i.e. match the children of the current route. Then the parent component could more easily setup a nav linking to its children using simply bar and baz. It would require some additional way to link to siblings though, e.g. bar could link to baz using ../baz. AFAICT, this would be similar to how angular-ui-router does it.

taion commented 8 years ago

For queries and hashes I think we'd be better served with #2186 - it's separable from relative path links IMO because those two are non-hierarchical.

The ../ case is interesting. Imagine your routes look like:

<Route path="/a/b">
  <Route path="c">
</Route>

Assuming you're on /a/b/c, I think a link to d should take you to /a/b/d.

So far so good, but where should a link to ../d take you in that scenario? What about a link to d when you're on /a/b? I'm inclined toward both taking you to /d just to match the route declaration, but e.g. /a/d might be reasonable too? Not sure.

The reason I want to say "route definition" rather than current pathname is because that's the only definition from which it's immediately obvious that we should treat e.g. /a/b/c or /a/b/c/ identically in the example above.

agundermann commented 8 years ago

Good point. I think /d makes a lot more sense. If you really wanted to link from /a/b to /a/d, you could just put them into a common parent /a, couldn't you?

taion commented 8 years ago

Exactly! But it's a slight difference from how relative links would work "normally", e.g. per the RFC that @knowbody linked.

Neta-Alon commented 8 years ago

I'll try summarizing the behavior you guys thought of:

"Owner" Route Current Path Link Path Result Path
/a /a b /b
/a /a/b c /c
/a /a/b/c/d/e/f/g h /h
/b /a/b c /a/c
/b /a/b/c d /a/d
/c /a/b/c d /a/b/d
/a /a/b ../c /c (Interesting)
/b /a/b ../c /c
/c /a/b/c ../../c /c

So far it looks pretty good, maybe just this falls a bit short:

"Owner" Route Current Path Link Path Result Path
/a /a/b ? /a/c

But I'm guessing sibling-only relative path is pretty good. (this.props.location.pathname can also help here in making pseudo-relative paths e.g. <Link to={this.props.location.pathname + '/childRoute'}>).

Here it gets confusing:

<Route path="/a/b">
  <Route path="c">
</Route>
"Owner" Route Current Path Link Path Result Path
/c /a/b/c ../d /a/d ? /d
/a/b /a/b d /a/d ? /d

I agree on "route definition", it makes sense. But if we're going by that, /a/b is one unit to us ((/a/b) => /a) and therefore both examples above should go to /d. However - shouldn't you have some way of reaching /a/d relatively? Maybe it's not possible until the router can distinguish between /a and /a/.

Let me know if some cases are missing, I tried to think of them all.

colllin commented 8 years ago

Why not differentiate sibling from child by to="./d" or to="d".

I only have a few weeks experience with react-router, but I found it's design around absolute paths to be stubborn but responsible. A URL is a bit of a contract between the application and the user, and forcing everything to be absolute encourages the maintenance of these contracts (by making it more difficult to break them).

ryanflorence commented 8 years ago

Lets say we could break some existing behavior, like treating /a/ and /a differently, does that make speccing this out simpler? Does it let us follow the existing html spec around relative links?

taion commented 8 years ago

It would probably make things better and avoid potential nasty edge cases.

I think the big challenge is just figuring out "what should relative links mean in a nested routing context". Being able to define a relative link to the sub-path that a route component is on seems like it would be a lot more useful than interpreting all links relative to the full path.

agundermann commented 8 years ago

Yeah, I still don't see the point in implementing document-relative links. I see that they're useful for links in static html sites so that you can move directories around without breaking their internal links. That works because the URL is always the same for the same html file, thus href="help.html" always refers to help.html in the same directory. With react-router, route components (except leaf nodes) can be rendered in different directories, however.

I think links relative to the route in the rendering context are much better suited to solve the use case of writing more re-usable routes/components whose links don't break when you move them around.

timdorr commented 8 years ago

@taurose If you have a generic list component that you re-use, then relative links are really helpful. You can make the link to each list item just {id} and re-use it everywhere. I'm running into this with the more CRUDy parts of the apps I'm building. I'm having to build separate connector components that nest the generic list component, but it feels like a lot of boilerplate to me.

agundermann commented 8 years ago

I'm not sure I follow. Are you talking about document-relative links?

My point is that with a route config like this

<Route path="/" component={Parent}>
  <Route path="1" component={Comp1} />
  <Route path="2" component={Comp2} />
  ...
</Route>

you could set up a navigation in Parent using links relative to the current route, i.e. <Link to="1" />, or link from one inner component to another:: <Link to="../2" />.

With document-relative links and the existing spec, you could get there, too, using just <Link to="1"/> but things start falling apart if you

So ultimately, if you wanted to use document-relative URLs in a route with children, you'd always have to consider all reachable directories beneath that route and make sure they contain all the routes you link to.

With the other approach for resolving relative links, you don't have any of those problems, and <Link to="1" /> from Parent would always resolve to /1. And it's just as reusable, ie you could move the route definition around, use it in different places etc.

ryanflorence commented 8 years ago

@taurose imagine a pluggable authentication library complete with routes.

<Router>
  <Route path="/auth">
    {AuthLib.routes}
  </Route>
  {/* ... */}
</Route>

AuthLib could have internal links that are relative to an application-specific /auth path. Some other app could have it at /session etc.

One goal of the router I haven't really talked about is a "rails engine" type of system where lots of smaller apps can be built up with just route config. Relative links are paramount for such a system.

<Router>
  <Route path="/auth" children={Auth.routes}/>
  <Route path="/discussions" children={DiscussionApp.routes}/>
  <Route path="/chat" children={ChatApp.routes}/>
  <Route path="/courses" children={CourseApp.routes}/>
</Route>
ryanflorence commented 8 years ago

Side note: after the next release, relative links are probably the highest priority thing for me personally.

timdorr commented 8 years ago

@taurose What @ryanflorence posted was more of what I was thinking. Sorry, I was on my phone at the doctor's office waiting for an appointment, so I didn't have time to type out an example.

agundermann commented 8 years ago

@ryanflorence Huh? I never doubted the usefulness of relative links in general. I'm just saying, in response to taions comment, that I don't see any point in document-relative links (ie. relative to the browser's current full path) as opposed to "route-context relative" links (ie. relative to the active path segment of the route the component belongs to).

taion commented 8 years ago

:+1: to what @taurose says - I think we should seriously consider having our relative links be relative to the "route context". This lets me make a link to e.g. "bar" from <Route path="foo" component={FooComponent}> and not have to worry about it doing something different if a dev later embeds a child route under that route.

ryanflorence commented 8 years ago

sorry @taurose I misread.

I'd like to think about what happens when you need to change a route's path (either adding or removing / "segments") and what that does to the relative links it renders, and what changes the developer has to make..

taion commented 8 years ago

@ryanflorence The other complexity is that route segments may not match path separator boundaries, e.g. <Route path="/foo/bar">. Ick.

ryanflorence commented 8 years ago

Yeah, which is why (without a ton of thought) it seems like we make it relative to the url, not the route hierarchy.

christopherthielen commented 8 years ago

FWIW, ui-router relative links are relative to the state in which the link was created.

Given ui-router states: foo, foo.bar, foo.bar.baz, foo.bar.qux

If the view for foo.bar links to .baz, that link always links to foo.bar.baz, even if foo.bar.qux is currently active.

Likewise, if the view for foo.bar.baz links to ^.qux, that link always links to foo.bar.qux even if foo.bar.baz.somesubstate is currently active.

timdorr commented 8 years ago

ui-router uses named routes (states) as a primary mechanism for addressing, so they have the flexibility to do weird syntactic things like that.

We should follow standard URL syntax. E.g., ../foo would go up one in the chain.

ericclemmons commented 8 years ago

Found this one via the Googles. Ran into this situation myself as I have several "pluggable" routes (think, express().use(...)-style) that, depending on how Webpack builds, generates custom route structures based off of user roles.

For example, I have the URL:

When rendering GettingStarted.js (who matched path: "getting-started"), I have <Link to="installation">, which shows up as /installation. Color me surprised :)

Has anyone figured out a solution for this, or is the debate still on?

taion commented 8 years ago

I don't think we're going to have anything for 2.0.0 unless a PR materializes out of nowhere.

To make this work in a reasonable way, I think we need to at least add a context provider component that tells everything under a given route which route it's under.

ericclemmons commented 8 years ago

Yep, that's ultimately what I got working tonight. Pretty much re-introducing childContextTypes.location to do a url.resolve(location.pathname + "/", to).

Preliminary effort packaged up as a <RelativeLink> in my project, but I know there was a reason for dropping location, route, etc. from child contexts...

ericclemmons commented 8 years ago

Ok, I got something more sophisticated working in a work project tonight.

  1. I'm enhancing each route component with childContextType.route via a HoC.
  2. Additionally, each childRoute gets a parent attribute that references the parent route.
  3. Next, I have my own <Link> component that leverages context.route to traverse the parent paths.
  4. It reduces these paths using url.resolve in such a way that absolute paths stay absolute, and relative to props are built based on who it descends from.

This ended up being much better than relying on location.

This is a bi-product of having state-aware routes, which I'll be publishing here in the near future:

ericclemmons/react-router-enhanced-routes#1