Ensuring stable sort of route rules used for matching URLs. #2502
Conversation
cdriscol
force-pushed
the
stable_sort_fix
branch
from
cb8b751
to
a8d02eb
Compare
cdriscol
force-pushed
the
stable_sort_fix
branch
from
a8d02eb
to
72f1da0
Compare
| @@ -59,7 +59,31 @@ describe("UrlRouter", function () { | |||
| match = ['/foo/bar', $match]; | |||
| }).when('/bar', function($match) { | |||
| match = ['/bar', $match]; | |||
| }); | |||
| }).when('/', function() { | |||
There was a problem hiding this comment.
These rule definitions may seem abundant - but it was the only way I could consistently reproduce the stable sorting issue in Chrome.
|
Wow, nice! Thanks for doing the research on this. We were just gonna roll back #1585, but this seems like a better solution for sure. /cc @christopherthielen |
| @@ -308,7 +309,8 @@ function $UrlRouterProvider( $locationProvider, $urlMatcherFactory) { | |||
| rules.sort(function(ruleA, ruleB) { | |||
| var aLength = ruleA.prefix ? ruleA.prefix.length : 0; | |||
| var bLength = ruleB.prefix ? ruleB.prefix.length : 0; | |||
| return bLength - aLength; | |||
| var lengthDiff = bLength - aLength; | |||
| return lengthDiff !== 0 ? lengthDiff : ruleA.position - ruleB.position; | |||
There was a problem hiding this comment.
Ensures a stable sort in Chrome browsers
|
I really do like the idea of ordering the rules by specificity by default. We should definitely do that for 1.0. My main concern with #1585 is that it's a legitimate breaking change from 0.2.15. For years we've told people that url rules are checked in the order that they were registered, and that they should re-order the registration of those rules if they need to change match priority. Because sorting rules was a BC for 0.2.x, however, I think we should do one of three things for 0.2.18:
|
|
Yeah, fair enough. @cdriscol Update the PR to point to |
|
could this fix be released with a quick minor update soon? |
|
@nateabele - I will get this into the will be good to get the sort rolled back in 0.2.18 |
|
It would be good to release a new version for this soon. I found that it breaks a lot of stuff |
|
@sebastiannm Yup, that's why it's going into 1.0 only, and the previous version is being rolled back. |
|
@nateabele When do ya think the 0.2.18 release will come out? Just noticed this broke my app so had to revert to .15 |
|
@JetFault Hard to say. We have this and one or two other BC breaks to patch up. |
This is the fix for #2501.