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

Build up specificity for nested routes. #103

Merged
merged 1 commit into from
Aug 12, 2016
Merged

Build up specificity for nested routes. #103

merged 1 commit into from
Aug 12, 2016

Conversation

tjni
Copy link
Contributor

@tjni tjni commented Aug 4, 2016

When adding an array of routes, each route will increase the
specificity instead of using only that of the last route.

Resolves #102.

@mixonic
Copy link
Collaborator

mixonic commented Aug 5, 2016

@tjni you are on the right track here. Specificity of a previously added route was being stomped by a second route, and using an object as a bucket avoids the stomping. However I'm not sure this restores all the functionality we need (there are definitely two tests I have added that do not pass with it) however this is a step in the right direction.

@mixonic
Copy link
Collaborator

mixonic commented Aug 5, 2016

@tjni this seems an incremental fix, but fails on the following assertion:

test("Matches the route with the longer static prefix with nesting", function() {
  var handler1 = { handler: 1 };
  var handler2 = { handler: 2 };
  var router = new RouteRecognizer();

  router.add([{ path: "/", handler: handler1 }, { path: ":post_id", handler: handler1 }]);
  router.add([{ path: "/team", handler: handler2 }, { path: ":user_slug", handler: handler2 }]);

  resultsMatch(router.recognize("/team"), [
    { handler: handler2, params: { }, isDynamic: false },
    { handler: handler2, params: { }, isDynamic: false }
  ]);
});

Which I believe is appropriate to assert emberjs/ember.js#13960 is fixed.

@tjni
Copy link
Contributor Author

tjni commented Aug 5, 2016

Planning on looking into this in a few hours in the evening, unless someone else is already.

@mixonic
Copy link
Collaborator

mixonic commented Aug 5, 2016

@tjni in fact the test you've added here already passes on master, I believe.

@mixonic
Copy link
Collaborator

mixonic commented Aug 5, 2016

scratch that, my test does not pass in 0.1.5 either. I must have incorrect ported emberjs/ember.js#13960

@mixonic
Copy link
Collaborator

mixonic commented Aug 5, 2016

@tjni I've confirmed you fix with this test: https://github.com/tildeio/route-recognizer/pull/105/files#diff-d4ce617de337dd77d23737176f335d8cR884 Please add it to this PR! I believe that will give us the option for moving forward with this change instead of reverting.

mixonic added a commit to mixonic/route-recognizer that referenced this pull request Aug 6, 2016
Route specificity was being limited to only that of the last segment.
This was incorrect causing some regressions in Ember.

Original PR at: tildeio#103
When adding an array of routes, each route will increase the
specificity instead of using only that of the last route.
@nathanhammond
Copy link
Contributor

LGTM

@@ -151,8 +148,6 @@ function parse(route, names, specificity, shouldDecodes) {
}
}

specificity.val = +specificity.val;
Copy link
Contributor

@nathanhammond nathanhammond Aug 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unnecessary type conversion which is why it was removed.

@krisselden krisselden merged commit de4d0fb into tildeio:master Aug 12, 2016
@nathanhammond nathanhammond deleted the specificity branch August 12, 2016 21:36
@nathanhammond
Copy link
Contributor

nathanhammond commented Aug 16, 2016

@mixonic It turns out we did need to land the test from #85.

I've extracted exactly the routes generated from emberjs/ember.js#14073 and we trigger the same failure as the test from #85.

Here is what Ember generates to feed into route-recognizer:

  router.add([{"path":"/application_loading","handler":"application_loading"}], {"as":"application_loading"});
  router.add([{"path":"/_unused_dummy_error_path_route_application/:error","handler":"application_error"}], {"as":"application_error"});
  router.add([{"path":"/","handler":"application"},{"path":"/loading","handler":"loading"}], {"as":"loading"});
  router.add([{"path":"/","handler":"application"},{"path":"/_unused_dummy_error_path_route_application/:error","handler":"error"}], {"as":"error"});
  router.add([{"path":"/","handler":"application"},{"path":"/test1_loading","handler":"test1_loading"}], {"as":"test1_loading"});
  router.add([{"path":"/","handler":"application"},{"path":"/_unused_dummy_error_path_route_test1/:error","handler":"test1_error"}], {"as":"test1_error"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/loading","handler":"test1.loading"}], {"as":"test1.loading"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/_unused_dummy_error_path_route_test1/:error","handler":"test1.error"}], {"as":"test1.error"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/test2_loading","handler":"test1.test2_loading"}], {"as":"test1.test2_loading"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/_unused_dummy_error_path_route_test2/:error","handler":"test1.test2_error"}], {"as":"test1.test2_error"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/test2","handler":"test1.test2"}], {"as":"test1.test2"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/index_loading","handler":"test1.index_loading"}], {"as":"test1.index_loading"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/_unused_dummy_error_path_route_index/:error","handler":"test1.index_error"}], {"as":"test1.index_error"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/","handler":"test1.index"}], {"as":"test1.index"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/","handler":"test1.index"}], {"as":"test1"});
  router.add([{"path":"/","handler":"application"},{"path":"/","handler":"test1"},{"path":"/","handler":"test1.index"}], {"as":"application"});
  router.add([{"path":"/","handler":"application"},{"path":"/misc_loading","handler":"misc_loading"}], {"as":"misc_loading"});
  router.add([{"path":"/","handler":"application"},{"path":"/_unused_dummy_error_path_route_misc/:error","handler":"misc_error"}], {"as":"misc_error"});
  router.add([{"path":"/","handler":"application"},{"path":"/:param","handler":"misc"}], {"as":"misc"});

The adjacent epsilon segments break things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants