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

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

Closed
michaelAndersonCampingWorld opened this issue Feb 3, 2017 · 23 comments
Labels
Milestone

Comments

@michaelAndersonCampingWorld

React Router 4.0.0-beta.4
I ran into this today, and it was quite confusing.

Here is my app:

const Header = ({ match}) =>
  <h1>
     <Route exact path="/" component={Home} />
     <Route path={`${match.url}/account`} component={Account} />
  </h1>

<Router>
  <Route path="/" component={Header}
</Router>

In this case, the path for the Account component will only match when I've come from a place where match.url is not /. When match.url is / then that route path is //account and it doesn't match. For other paths, I'd be able to have a convention, where I never end my route paths with /, for instance if match.url is /user then Account path is /user/account which will match.

@mjackson
Copy link
Member

mjackson commented Feb 3, 2017

Hmm, ya, we should fix this. Good catch :)

I don't think the fix is to treat // and / the same way though. That will probably just lead to more problems further down the road.

There are a couple of things we could do here:

  • We could special-case match.url to be an empty string when we match the root URL. Since match.url is provided explicitly for the purpose of building route paths, I think this is OK.
  • We could make <Route> (and probably <Link>) tolerant of strings that begin with //, knowing that this would happen when building route paths from the root URL.

Related: I think the relevance of match.url (and match.path) are probably greatly reduced if we can figure out relative <Route>s and <Link>s in #4355

@michaelAndersonCampingWorld
Copy link
Author

From my perspective, I think that the second solution is one that is easier to understand. It might be confusing that match.url is empty on the root url, but prominent documentation on it might be able to sort it out for new users.

However, I agree with you that this is much smaller issue if relative paths are handled gracefully.

@mjackson
Copy link
Member

mjackson commented Feb 5, 2017

Related: #4459

@timdorr timdorr added the feature label Feb 6, 2017
@timdorr timdorr added this to the v4.0.0 milestone Feb 6, 2017
@earnubs
Copy link

earnubs commented Feb 9, 2017

Warning: React attempted to reuse markup in a container but the checksum was invalid.
[snip]
(client) actid="4"><a href="/home" data-reactid="
 (server) actid="4"><a href="//home" data-reactid=

Think I've come across the same thing?

@timdorr
Copy link
Member

timdorr commented Feb 9, 2017

@earnubs That will be fixed by #4484, when released.

@earnubs
Copy link

earnubs commented Feb 9, 2017

@timdorr thanks!

@carsonxyz
Copy link

Thanks for the quick fix. Very eager for this fix to be released. Preventing SSR from working for me after spending time migrating lots of code from alpha releases.

@leebenson
Copy link

Ran into the same issue.

Snippet of component code...

<ul>
  <li><Link to="/">Home</Link></li>
  <li><Link to="/page/about">About</Link></li>
  <li><Link to="/page/contact">Contact</Link></li>
</ul>
<hr />
<Route exact path="/" component={Home} />
<Route path="/page/:name" component={Page} />``

And doing SSR via Koa:

const route = {};
const html = ReactDOMServer.renderToString(
  <StaticRouter location={ctx.request.url} context={route}>
    <App />
  </StaticRouter>,
);

On the client, the 'About' link is /page/about. On the server, it's //page/about.

Of course, // is trying to reuse the original protocol, so clicking the link tries to find a host named page rather than a relative route.

Currently all SSR routes are broken.

@leebenson
Copy link

What's the ETA for the release? Thanks in advance.

@pshrmn
Copy link
Contributor

pshrmn commented Feb 12, 2017

This issue concerns manually building URLs by concatenating match.url with a "relative" path.

This issue is not related to the <StaticRouter> producing links that begin with two slashes.

Please only comment about the former. The latter has been fixed and should be released soon 😄

@leebenson
Copy link

Sorry @pshrmn, that wasn't clear. On first glance, the OP's syntax looked near identical to my own, so I thought this was probably the same underlying thing.

Do you have any ETA for the release?

@timdorr
Copy link
Member

timdorr commented Feb 13, 2017

There are no ETAs. This is beta software, so you use at your own risk. Sorry to sound harsh, but you definitely should not be using this code in production and can't rely on fixes being released in a timely manner.

But that being said, I would expect one by the end of the month for sure.

@leebenson
Copy link

leebenson commented Feb 13, 2017

@timdorr, then might I respectfully suggest you make that clear on https://reacttraining.com/react-router or the v4 README?

When I ask for ETAs, it's not to place demands on your time or to suggest you're beholden to anyone. It's because I'm excited about the release, and this is a very small detail that is preventing a major part of the framework's promised benefit.

The 'beta' warning in this issue is the first I've seen. It's not mentioned in the readme. The training site has a ton of (very useful) syntax, and the API seems complete. There's even a video showing it working in practice. That's far more than most projects that limp by in 'beta' for years, and accrue plenty of production users. Node itself was pre-1.0 for 6 years. I'm not sure the casual 'beta' tag is enough of a deterrent in the JS ecosystem.

For my use case personally, I'm building a starter kit that puts v4 front and centre. On the react-hot-loader starter kit page, there are multiple projects listed that feature react-router 4... clearly, I wasn't alone in thinking this was ready for the wild. If v4 isn't ready, I'll obviously need to re-write. Seeing a warning not to use it would have saved me a few days.

I certainly don't wish to dictate your release schedule, and I appreciate all of your hard work on this awesome package. But if it's already fixed, is there any way you could bump the patch to a new release and help out me and @carsonperrotti and others for whom this simple fix (that has already been merged!) is actually a pretty major roadblock to SSR?

@verekia
Copy link

verekia commented Feb 13, 2017

In the meantime, a crutch fix can be done with a regex replacement:

serverSideHTML.replace(/<a href="\/\//g, '<a href="/')

That should make both your server-side HTML and client-side HTML match but note that you will still get the warning for checksum mismatch.

Looking forward to the release! :)

@leebenson
Copy link

that's actually almost exactly what I wound up doing - good tip, @verekia!

@mjackson
Copy link
Member

@leebenson Thanks for the input. I'm the one who cuts releases from the v4 branch, so you can blame me for the slow release. We're keeping busy and pushing steadily forward. Thanks for your patience :)

@mjackson
Copy link
Member

OK, 4.0.0-beta.6 is released.

That's far more than most projects that limp by in 'beta' for years, and accrue plenty of production users.

Clearly, you never used Gmail ;)

@verekia
Copy link

verekia commented Feb 14, 2017

Thank you very much @mjackson! beta.6 indeed fixed it! :)

@leebenson
Copy link

Thanks @mjackson!

@leebenson
Copy link

Clearly, you never used Gmail ;)

That's exactly my point. 'Beta' doesn't deter usage.

Half of my stack is alpha/beta/pre-1.0. What's ever really production-ready in the JS world?

If the official suggestion is for users to consider this dev-only, I'd definitely make that clearer.

But so far, I have to say this was the only real issue I came across and that's been released- so thanks 😄

@mjackson
Copy link
Member

@leebenson You're welcome :)

Side note: your life as a cutting-edge software dev will be greatly simplified if you develop strategies for using not-yet-released work so you can move on with your development w/out having to bug the maintainers to cut a release. Yes, I know it's more convenient to just npm install something. But in the meantime you can use npm link to link a local copy of stuff on your machine. Or, if you really need a package.json that you can share, you can try publishing your own fixed version of the package in a github branch and then linking to it from your package.json. I do this all the time. In fact, I'm doing it right now in the website package.

@leebenson
Copy link

@mjackson - that's definitely good advice. Sorry if I was a PITA on this occasion, but I do appreciate the fast release. You've done an awesome job with v4.

@ryanflorence
Copy link
Member

seems like this should be closed? please reopen if I'm wrong.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 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

9 participants