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

Relative <Link to> #2172

Closed
Robinfr opened this issue Oct 6, 2015 · 77 comments
Closed

Relative <Link to> #2172

Robinfr opened this issue Oct 6, 2015 · 77 comments
Labels

Comments

@Robinfr
Copy link

Robinfr commented Oct 6, 2015

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 mjackson changed the title Relative Link Relative <Link to> Oct 6, 2015
@mjackson
Copy link
Member

mjackson commented Oct 6, 2015

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

@knowbody
Copy link
Contributor

knowbody commented Oct 6, 2015

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
Copy link
Member

mjackson commented Oct 6, 2015

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
Copy link
Contributor

knowbody commented Oct 6, 2015

oh yeah! I might work on that tomorrow

@jussi-kalliokoski
Copy link

👍 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:

  • /orders/new contains navigation of its children
  • /orders/new/contact-details
  • /orders/new/billing-information
  • /orders/new/confirmation
  • /dashboard/orders/new
  • /dashboard/orders/new/contact-details
  • /dashboard/orders/new/billing-information
  • /dashboard/orders/new/confirmation

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
Copy link

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
Copy link
Member

mjackson commented Oct 7, 2015

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

taion commented Oct 14, 2015

Wouldn't you do something like

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

?

@qimingweng
Copy link
Contributor

@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
Copy link
Contributor

taion commented Oct 14, 2015

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

@qimingweng
Copy link
Contributor

@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
Copy link
Contributor

taion commented Oct 14, 2015

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

@qimingweng
Copy link
Contributor

@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
Copy link
Contributor

taion commented Oct 14, 2015

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
Copy link
Contributor

@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
Copy link
Contributor

taion commented Oct 14, 2015

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
Copy link

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
Copy link
Contributor

taion commented Nov 7, 2015

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
Copy link
Contributor

taion commented Nov 10, 2015

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
Copy link
Contributor

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
Copy link
Contributor

taion commented Nov 11, 2015

Two problems:

  • The router treats /foo and /foo/ as being effectively effectively the same, but per the RFC a link to bar for the former would go to /bar, while for the latter would go to /foo/bar; I don't think anybody wants this inconsistency
  • Given our concept of nested routes, it's not clear that you want to route this way - in the example I have above, if I declare a relative link on <Parent>, it isn't obvious that it should be resolved relative to the full path (rather than the path segment associated with that <Parent> element)

@ryanflorence ryanflorence added this to the next-2.4.0 milestone Apr 14, 2016
@ryanflorence
Copy link
Member

We're bringing this to the router, work has started over here and will be brought into core when the kinks are worked out over there.

@frontsideair
Copy link
Contributor

This just bit me today. I gave a relative address to Link element without noticing and wondered what was wrong when it rendered the wildcard route. Curiously, it rendered the correct route when I used browser back button and then went forward.

@taion
Copy link
Contributor

taion commented May 31, 2016

As part of this, we will want to put route on context.router.

@taion
Copy link
Contributor

taion commented Jun 30, 2016

I am dropping this from the v3 milestone – while I very much think we should have this feature, I don't think we can treat this as a v3 blocker.

@taion taion modified the milestones: next-sometime, next-3.0.0 Jun 30, 2016
@firasdib
Copy link

I've read through this entire issue and I don't see what the problem is. Now I have to use something like https://www.npmjs.com/package/resolve-url and feed it resovleUrl(window.location.pathname, '../') which feels like something that should be natively supported. Could someone ELI5?

@Robinfr
Copy link
Author

Robinfr commented Jul 13, 2016

I must say guys, even though I don't entirely agree with not following how a normal link works, I think amazing work is being done!

@shaibam
Copy link

shaibam commented Aug 14, 2016

So... is there gonna be a way to pass a relative link in the 'to' field or not?

@taion
Copy link
Contributor

taion commented Aug 14, 2016

This is a feature that we want to support. I'm not aware that anybody is actively working on it, though.

@optimatex
Copy link

I'm sorry guys but after reading i do not understand what is already done and what is in plans.
the variant <Link to="../docs" /> doesn't work for me.
As i understand the only way to realize relative link is
<Link to={${this.context.location.pathname}/picture/${id}} > - at least it works for me

@txm
Copy link

txm commented Aug 31, 2016

This is my "solution" :/

function splitURL(href) {
return href.split('?')[0].split('#')[1];
}

let url = splitURL(window.location.href) + '/' + this.props.value;

@just-boris
Copy link

@txm look at solution by @optimatex, it is better because uses only react-router internals, and doesn't rely on global window object

@timdorr
Copy link
Member

timdorr commented Sep 13, 2016

Works in v4 :) https://react-router-website-uxmsaeusnn.now.sh/recursive-paths

@Blackclaws
Copy link

Blackclaws commented Nov 5, 2016

Scratch my complaint. I was still under the impression that the resolution was workable for v2/v3 at some point.

All in all does this mean that it won't be supported in the old v2/v3 branch ever but will only be available using the v4 Match syntax?

@saiichihashimoto
Copy link

I was under the assumption that v2/v3 was to be supported

@mjackson
Copy link
Member

mjackson commented Nov 5, 2016

@saiichihashimoto v3 will receive bugfixes but no new features. Part of adding this particular feature meant that the code needed to be refactored in backwards-incompatible ways.

@adamdonahue
Copy link

adamdonahue commented Aug 14, 2017

Realize this is old, but is there a solution to this yet?

Assume a path like:

/deals/:dealId/trades/:tradeId/notes/:noteId.

Further assume we're using tabs at each level.

In order to link from one note to another, then, I need that component to know the context in which it exists.

<Link to={`/deals/${dealId}/trades/${tradeId}/notes/${noteId}`}>Note 1 Tab</Link>

and so forth. So a note needs to know its context when in fact it shouldn't. Is there an elegant pattern around this?

@timdorr
Copy link
Member

timdorr commented Aug 14, 2017

See #5127

@remix-run remix-run locked and limited conversation to collaborators Aug 14, 2017
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