Relative <Route> and <Link> #4355

pshrmn · 2017-01-19T23:02:04Z

Re-pulling this for the new v4, if for nothing else than as a proof of concept for relative path support. This adds support for both <Link>s (actually I just created a <RelLink>, but <Link> could be adapted) as well as <Route>s and <Switch>es.

This supports relative URL resolving (described by RFC 3986 ) with the difference being that it does not strip off the last URL segment of the base pathname (unless there is a trailing slash).

// with RFC 3986
resolve('three', '/one/two') // '/one/three'
// keeping last segment
resolve('three', '/one/two') // 'one/two/three'

This is important because it allows a <Link> to be relative to its parent match.url.

// with RFC 3986
<Route path='/one/two' render={() => (
  <Link to='three'>Three</Link> // <a href='one/three'>Three</a>
)} />
// keeping last segment
<Route path='/one/two' render={() => (
  <Link to='three'>Three</Link> // <a href='one/two/three'>Three</a>
)} />

There are some assumptions made in the resolution process, namely that the base pathname is only a pathname (no protocol, domain, search string or hash fragment).

I adapted the tests provided by RFC 3986, so I am fairly confident in it, but I'd appreciate breaking tests if I missed any situations.

Quick Example

const App = () => (
  <div>
    <Header />
    <Route path='parent' component={Parent} />
  </div>
)

// the parent match of Header is null, which will resolve
// pathnames with an empty URL segment
// 'parent' will resolve to '/parent'
const Header = () => (
  <header>
    <RelLink to='parent'>Parent</RelLink>
  </header>
)

// the parent match of Parent is { url: '/parent', ... }
// so pathnames will resolve with '/parent'
// 'one' will resolve to '/parent/child'
const Parent = () => (
  <div>
    <RelLink to='child'>Child</RelLink>
    <Route path='child' component={Child} />
  </div>
)

const Child = () => (
  <div>
    <p>This is the child component</p>
    <RelLink to='..'>Back</RelLink>
  </div>
)

render((
  <BrowserRouter>
    <App />
  </BrowserRouter>
), holder)

mjackson · 2017-01-20T15:24:52Z

modules/resolve.js

+  if (typeof location === 'string') {
+    return resolve(location, base)
+  } else {
+    location.pathname = resolve(location.pathname, base)


We're already using https://github.com/mjackson/resolve-pathname in the history lib. I think it makes sense to re-use it here?

I tested out resolve-pathname by adding a noncompliant mode to it: resolvePathname(to, from, noncompliant). Essentially all that it does is to not pop the filename (https://github.com/mjackson/resolve-pathname/blob/master/modules/index.js#L26) when in that mode. I also added the modified examples from RFC 3986 to test this out. There were a couple of issues:

It does not work when to includes a hash or search string. I would expect resolvePathname('?x=y', '/a/b', true) to resolve to /a/b?x=y, but it is currently including a trailing slash and resolving to /a/b/?x=y. This also fails for abnormal examples like g#s/../x resolving to /a/b/c/d/x. I was dealing with that by checking for the earliest index of ? or #, stripping that index to the end of of the string off, resolving, then appending that to the resolved pathname before returning. It is easy to add that functionality to resolvePathname assuming you want to support that.

There are some dot and double-dot notation issues. It gets a little bit complicated because this isn't RFC compliant, but I feel that resolvePathname('..', '/a/b/c/d', true) should resolve to /a/b/c while it is resolving to /a/b/c/.

It does not work when to includes a hash or search string.

resolve-pathname is designed to resolve only the pathname portion of the URL. Leaving out search and hash was intentional. As you say, stripping and re-adding that stuff after the pathname is correctly resolved is the easy part :) I think I'd rather do that in the router codebase somewhere and just let resolve-pathname do the one thing it's good at.

As for non-compliance with the RFC, I agree with you. Having a <Route path="c"> nested under a <Route path="/a/b"> should match /a/b/c. However, I think we should still be able to use resolve-pathname for that by just adding a trailing slash to the parent route's path before passing it in, e.g. resolvePathname(parentMatch.path + '/', path).

Adding a trailing slash should work, not sure why I didn't think of that before.

I think that I will need to add a check so that resolving .. with /a/b/c returns /a/b and not /a/b/, but otherwise no issues with that comes to mind.

mjackson · 2017-01-20T15:30:35Z

Thanks for another great PR, @pshrmn. We've talked a lot about including support for relative <Route>s and <Link>s in the router over the years, but we've always held off for fear of not getting it exactly right. I think with v4 the code may finally be at a place where we can experiment more easily, as you've shown in this PR.

One thing I'd like to make sure we remember to do is to keep support for absolute <Route>s and <Link>s that start at the root URL. For example, a <Route path="/a/b"> should always match the root URL, regardless of where it's nested. I'd like to see a test for that.

Also, I think we may be able to leverage the work I've done in https://github.com/mjackson/resolve-pathname. Seems like some of this is duplicating what I've already done there.

As far as including better support for relative routes/links in the router, we're very keen to do so. As with everything, just need to be careful we get it right when we do ;)

How would you feel about working up <RelativeRoute> and <RelativeSwitch> as proofs of this concept, alongside <RelativeLink>? If we had all 3 then we could experiment more easily and figure out if making everything relative should just be the default.

pshrmn · 2017-01-20T17:17:55Z

One thing I'd like to make sure we remember to do is to keep support for absolute <Route>s and <Link>s that start at the root URL. For example, a <Route path="/a/b">

I can add a test for that. The nice thing about relative <Route> paths is that we can guarantee that they will render given the correct URL. A misplaced absolute path may never render.

<Route path='/a' render={() => (
  // this will never be matched.
  <Route path='/b/c' component={BC} />
)} />

mjackson · 2017-01-20T17:36:50Z

We can actually emit a warning to let people know about that if we wanted to.

pshrmn · 2017-01-20T18:00:17Z

How would you feel about working up <RelativeRoute> and <RelativeSwitch> as proofs of this concept, alongside <RelativeLink>? If we had all 3 then we could experiment more easily and figure out if making everything relative should just be the default.

<Route> and <Switch> are already modified to work with relative paths in this PR. They use simpleResolve (which is really just a string concatenation that makes sure not to add an extra / in between the paths) to join the parent match.path with the current path before matching against the current location.pathname.

pshrmn · 2017-01-20T18:10:45Z

Really, I probably should have just modified the existing <Link> component too. As it is, <RelLink> doesn't work with <NavLink> because they both wrap <Link>. I was just nervous about the fact that every <Link> would then be subscribing to the history and whether that would impact render performance. I'm sure that it could be customized so that <Link> only subscribes when to is relative, but it was faster to make <RelLink> for this demo.

mjackson · 2017-01-20T18:45:46Z

OK, let's try incorporating it directly into <Link> instead.

mjackson · 2017-01-20T19:12:51Z

Just a heads up @pshrmn: everything got moved around in 4349de3. We're following the same "monorepo" design as React itself because we're going to start splitting things out into several different packages.

pshrmn · 2017-01-20T19:26:31Z

@mjackson Before I accidentally rebase and add a thousand files, the .gitignore is missing node_modules.

mjackson · 2017-01-20T20:18:47Z

@pshrmn I always add node_modules to my global .gitignore so I don't have to add it to every project I work on.

pshrmn · 2017-01-20T23:38:10Z

Added relative support to <Link>, so you should be able to play around with that. I'll go through the examples on the website later and try to make everything relative to see if anything breaks.

Right now the only thing that I would consider changing is that there is no default match (or rather, the default match is null). This requires checking whether match.path and match.url exist. It isn't a big deal, but having an object with base values to grab might make some of the code cleaner.

{ path: '/', url: '', params: {}, isExact: false? }

I had thought at first there was an error in resolve-pathname, but as I was comparing the RFC 1808 and 3986 examples I saw that they had changed the behavior of /../g, which resolve-pathname does handle correctly (and my version of resolve had not been 😊).

Side note: I had added node_modules to .gitignore in one of the earlier commits before you responded. I can remove it, but it is included in the v3 .gitignore and the packages/react-router-website .gitignore.

timdorr · 2017-01-21T02:00:09Z

@mjackson

@pshrmn I always add node_modules to my global .gitignore so I don't have to add it to every project I work on.

That works well with smaller projects, but on something this big (or if there was a larger team), it will result in problems for those who want to contribute and don't have that set up. I would really recommend adding it here.

timdorr · 2017-01-21T02:06:23Z

@pshrmn

Really, I probably should have just modified the existing <Link> component too. As it is, <RelLink> doesn't work with <NavLink> because they both wrap <Link>. I was just nervous about the fact that every <Link> would then be subscribing to the history and whether that would impact render performance. I'm sure that it could be customized so that <Link> only subscribes when to is relative, but it was faster to make <RelLink> for this demo.

Why not just resolve the link in handleClick rather than updating it in real time?

mjackson · 2017-01-21T03:54:41Z

@timdorr

Why not just resolve the link in handleClick rather than updating it in real time?

Because we need a real href in the DOM in order to make pages accessible. This has been one of our tenets from the beginning.

timdorr · 2017-01-21T03:57:51Z

The browser will resolve relative links in hrefs for you: http://jsbin.com/cugifakufo/edit?html,output What does that have to do with a11y? (Legit question, I'm not an expert in it)

pshrmn · 2017-01-21T04:16:25Z

@timdorr The pathnames in a <Link> aren't resolved relative to the actual URL, but to the part of the URL that was matched by the <Route> that it is in.

i.e.

const App = () => (
  <BrowserRouter>
    <div>
      <Link to='products'>Products</Link> // resolves to /products
      <Route path='products' render={() => (
        <div>
          <Link to='tables'>Tables</Link> // resolves to /products/tables
          <Route path=':item' render={() => (
            <Link to='..'>Products</Link>  // resolves to /products
          )} />
        </div>
      )} />
    </div>
  </BrowserRouter>
)

mjackson · 2017-01-21T04:28:18Z

As @ryanflorence said in #4153 (comment):

That's a relative path, and we don't really support that right now. (It's way more subtle and tricky to get to work than people think.)

This is a tricky problem to solve ;)

The function resolveLocation can be used for resolving location descriptors and strings. Resolving is done with a modified version of RFC 3986's resolution algorithm in which the trailing URL segment is not removed. This allows a <Link> to be resolved relative to the <Route> that is is located in. resolveLocation supports pathnames that include double dot notation (../foo). The function simpleResolve can be used for resolving <Route> and <Switch> pathnames. This simply joins the pathname (iff it is not absolute) to the base pathname using a forward slash.

The parent match is passed to matchPath. match.url is used for resolving instead of math.path to simplify matching (just match a string instead of having to parse params). matchPath will combine its parent's params with its params (favoring its params when there are conflicts)

pshrmn

I know that things are pretty hectic with the beta, but I think that this is in a pretty good place for a review @mjackson (and anyone else).

There appears to be a problem in the tests that I don't run into locally, so I'll need to figure that out, but otherwise everything seems to work for me.

pshrmn · 2017-02-03T20:05:27Z

packages/react-router-website/modules/components/ExampleRouter.js

+    }
+
+    this.unlisten = this.router.listen(() => {
+      this.router.location = this.context.router.location


I had to modify <ExampleRouter> in order for it to work with relative routes. The <FakeBrowser> renders everything inside of a pathless <Route>, so without this relative <Route>s and <Link>s would resolve using that <Route>'s match. This just sets the <ExampleRouter>'s match to null. It also has to listen for location changes because any of its children that listen for location changes would be re-rendering via the listener prior to <ExampleRouter>'s getChildContext being called on navigation.

pshrmn · 2017-02-03T20:06:23Z

packages/react-router/modules/Route.js

@@ -98,7 +104,7 @@ class Route extends React.Component {

  componentWillReceiveProps(nextProps) {
    Object.assign(this.router, {
-      match: computeMatch(this.router, nextProps)
+      match: computeMatch(this.context.router, nextProps)


Need to use the parent router object, not this.router in order to have access to the parent match.

This change makes me nervous. If it's needed here, it feels like the kind of thing that should be needed more generally. If it's not needed outside the context of this PR, it's probably going to cause a bug.

pshrmn · 2017-02-03T20:10:27Z

packages/react-router/modules/matchPath.js

-    return { url: pathname, isExact: true, params: {} }
+    return { url: parent ? parent.url : '/', isExact: true, params: {} }
+
+  path = simpleResolve(path, parent && parent.url ? parent.url : '')


Resolve using parent.url instead of parent.path for a simpler regexp comparison. Below, the parent's params are assigned to the params object so that the child can also reference them.

<Route path=':category' render={() => ( <Route path=':id' render={({ params }) => ( console.log(params), // { category: 'lamps', id: 'i<3' } null )} /> )} />

pshrmn · 2017-02-03T20:13:22Z

packages/react-router/modules/matchPath.js

  const { exact = false, strict = false } = options

  if (!path)
-    return { url: pathname, isExact: true, params: {} }
+    return { url: parent ? parent.url : '/', isExact: true, params: {} }


I had to change the url from pathname to whatever its parent match's url is because it otherwise was breaking relative matching. The full location is still available through the location prop. If someone is using a pathless <Route>, I'm not even sure why they would be checking the match.

pshrmn · 2017-02-03T20:15:17Z

packages/react-router/modules/matchPath.js

@@ -44,14 +47,19 @@ const matchPath = (pathname, path, options = {}) => {
  if (exact && !isExact)
    return null

+  const params = Object.assign({},
+    parent && parent.params,
+    keys.reduce((memo, key, index) => {


Mentioned above, but this merges parent params and the match's params into one object. I know that in v2/3, there were two different params objects, so I'm not sure if it would be preferred to return one object with only this path's params and one with both params merged.

mjackson · 2017-02-04T03:45:34Z

Either I'm just tired, or there's way too much going on in this PR to review all together. Do we need to do this all at once? Or are there pieces we can implement to get closer to where we want to be?

pshrmn · 2017-02-04T04:25:10Z

Most of it is tests, but perhaps that is all the more reason to split this up. I should be able to break this into three parts.

A resolve function (or two).
Components that automatically resolve themselves when relative.
Support for relative paths in the examples on the website.

pshrmn · 2017-02-04T07:23:35Z

Closed in favor or splitting this into multiple parts. Part 1 can be seen here: #4459

mjackson reviewed Jan 20, 2017

View reviewed changes

mjackson changed the title ~~resolve relative paths~~ Relative <Route> and <Link> Jan 20, 2017

pshrmn force-pushed the resolve branch from 4c7ce89 to 5778b2d Compare January 20, 2017 21:02

pshrmn force-pushed the resolve branch from 9d0b96a to 7e2ad3f Compare January 21, 2017 07:44

mjackson mentioned this pull request Jan 21, 2017

Empty relative path does not match index #4153

Closed

pshrmn force-pushed the resolve branch 7 times, most recently from 49f568b to e88acb2 Compare January 28, 2017 02:37

timdorr added the feature label Feb 1, 2017

timdorr added this to the v4.0.0 milestone Feb 1, 2017

mjackson mentioned this pull request Feb 3, 2017

Feature Request: Path should treat / and // as the same thing #4445

Closed

pshrmn force-pushed the resolve branch from e88acb2 to d6b1290 Compare February 3, 2017 18:45

pshrmn commented Feb 3, 2017

View reviewed changes

can't use pathless component in test

95b8aea

pshrmn force-pushed the resolve branch from 4a3b371 to 95b8aea Compare February 4, 2017 00:33

pshrmn closed this Feb 4, 2017

mjackson mentioned this pull request Feb 16, 2017

[WiP] Support relative paths in routes #4536

Closed

pshrmn deleted the resolve branch November 12, 2017 05:25

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relative <Route> and <Link> #4355

Relative <Route> and <Link> #4355

pshrmn commented Jan 19, 2017 •

edited

Loading

mjackson Jan 20, 2017

pshrmn Jan 20, 2017

mjackson Jan 20, 2017 •

edited

Loading

pshrmn Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

pshrmn commented Jan 20, 2017

mjackson commented Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

timdorr commented Jan 21, 2017

timdorr commented Jan 21, 2017 •

edited

Loading

mjackson commented Jan 21, 2017

timdorr commented Jan 21, 2017

pshrmn commented Jan 21, 2017 •

edited

Loading

mjackson commented Jan 21, 2017

pshrmn left a comment

pshrmn Feb 3, 2017

pshrmn Feb 3, 2017

mjackson Feb 4, 2017

pshrmn Feb 3, 2017

pshrmn Feb 3, 2017

pshrmn Feb 3, 2017

mjackson commented Feb 4, 2017

pshrmn commented Feb 4, 2017

pshrmn commented Feb 4, 2017

Relative <Route> and <Link> #4355

Relative <Route> and <Link> #4355

Conversation

pshrmn commented Jan 19, 2017 • edited Loading

Quick Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjackson Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

pshrmn commented Jan 20, 2017

mjackson commented Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

mjackson commented Jan 20, 2017

pshrmn commented Jan 20, 2017

timdorr commented Jan 21, 2017

timdorr commented Jan 21, 2017 • edited Loading

mjackson commented Jan 21, 2017

timdorr commented Jan 21, 2017

pshrmn commented Jan 21, 2017 • edited Loading

mjackson commented Jan 21, 2017

pshrmn left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjackson commented Feb 4, 2017

pshrmn commented Feb 4, 2017

pshrmn commented Feb 4, 2017

pshrmn commented Jan 19, 2017 •

edited

Loading

mjackson Jan 20, 2017 •

edited

Loading

timdorr commented Jan 21, 2017 •

edited

Loading

pshrmn commented Jan 21, 2017 •

edited

Loading