Support trailing slashes, not extraneous ones #3158
Conversation
|
Technically this is breaking, but IMO the old behavior is clearly not just wrong but actively buggy. |
|
Seems fine to me. This should be against |
|
This isn't documented. The intention is that this is safe for a patch-level release. |
|
But it's breaking, so semver dogmatics are going to be pissed. |
|
All bugfixes are technically breaking – this is more in that category, though. |
|
Updated the code to catch a case I was missing. |
|
Personally, I'm ambivalent about it. I don't think there's a practical problem with it and this looks mergable as-is. I'm just looking out for the purists, who will undoubtedly raise a stink. |
|
I need to improve this impl a bit. Don't merge this as-is. |
taion
changed the title
taion
changed the title
|
This is good to go now. |
|
I think we initially (unintentionally) added support for this in 6eba291 to, at a guess, better handle some edge cases with how we were combining route paths for purposes of building patterns (e.g. with training slashes and concatenating them, back when we did that). This is no longer relevant, so we can finally unwind all of that. |
| @@ -52,7 +52,7 @@ describe('v1 Link', function () { | |||
| Michael | |||
| </Link> | |||
| <Link | |||
| to="hello/ryan" query={{ the: 'query' }} | |||
| to="/hello/ryan" query={{ the: 'query' }} | |||
There was a problem hiding this comment.
This wasn't supposed to work, and now in fact it actually doesn't.
There was a problem hiding this comment.
To elaborate on "not supposed to work" – links like this will do random, dangerous things when used with e.g. browser history right now, given the lack of support there for relative links.
|
thanks @mxstbr .
|
|
Seems good, are there open issues associated with this fix? I'd like to see what problems it causes, also would like to see why @mjackson had it work the other way originally. Naively looking at the test cases I like the pull request's better. |
|
There's a small risk that this breaks someone. Less so the extraneous slashes bit, more with https://github.com/reactjs/react-router/pull/3158/files#diff-ee3bac4c318eeb54a00eedf5f9c2fd8aR55, though perhaps we can add some sort of workaround there to keep the old (but IMO buggy) behavior working. The main benefit is that this lets us make the |
|
Rebased. |
|
LGTM. Revert my merge if this looks terrible. |
|
Would you be okay with reverting this even if I don't think it looks terrible? I stand by my choice to call this a non-breaking bugfix, but all the same, I'd rather we release a v2.1.0 without this change first. |
|
(I probably should have noted that on the PR) |
|
I'd say rewind master locally, splice in a 2.1.0 commit and tag, and then replay this back on. No revert and revert-revert needed. |
|
It's a little ugly to release off master though. Then again, reverting and un-reverting is ugly too. FWIW, my thinking ATM is that we release master pre-this PR as 2.1.0, then resolve this PR plus deprecate nested absolute paths and do whatever we can w/link semantics for 2.2.0 (probably just optimize But I don't have npm access so I can't make this happen anyway. |
|
You can make the tags, so Ryan or Michael can check them out and publish easily enough. |
|
Not exactly – the version/publish cycle goes through our release tooling, so the tagging shouldn't be done separately from the release. See https://github.com/reactjs/react-router/blob/master/scripts/release.sh. And since the blocker is getting a release out, just tagging it isn't going to accomplish much anyway :p |
On further thought, the current behavior, despite being tested, is clearly just buggy. This will make life easier and opens the door to having a much simpler algorithm for checking isActive in the indexOnly sense.