Respect trailing slash when basename is used #8861
Conversation
|
Hi @senseibarni, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
|
React Router shouldn't care about trailing slashes, if it's there, leave it, if it's not, don't add it. @brophdawg11 you want to add to this PR to work that out and get some tests on it? |
ryanflorence
changed the title
|
👋 Thanks for the PR @senseibarni! I made some updates and corralled all the tests into a central location. I think this now properly handles being trailing-slash-agnostic for both |
| element={ | ||
| <> | ||
| <Link to="" /> | ||
| <Link to="/" /> |
There was a problem hiding this comment.
Aiming to leave trailing slash behavior in the hands of the user here and effectively treat paths as basename + link. If they do not include a trailing slash on their basename, then they can control the trailing slash behavior via <Link> such that <Link to="/"> will include the trailing slash and <Link to=""> will not.
If they do include a trailing slash on the basename, then it will be included on all links.
| <a | ||
| href="/app/" | ||
| />, | ||
| <a | ||
| href="/app/" | ||
| />, |
There was a problem hiding this comment.
When the basename has a trailing slash, routes to the root route will always have the trailing slash, but users still control trailing slash behavior on nested locations (see /app/parent/child below)
There was a problem hiding this comment.
Thank you for the review. getWindowImpl was missing part when I was trying to write some decent tests. Good to know how to manage such cases 👍
I am aware that the tests are not the best as they do not test actual code. The thing is that for this case the fully functional test is really hard to achieve. I tried with MemoryRouter and BrowserRouter. The real problem is only visible in the browser URL bar.
window.locationis not working well (because of the basename). Maybe someone smarter will come up with something or the test can be skipped as the changes don't look ambiguous.fix #8312
fix #6226