Update to route-recognizer (2.7.0) effects '/*wildcard' route

[derrickshowers]

Noticed a change in the /*wildcard route's behavior when added to the router map. As of 2.7.0 the wildcard route doesn't work as expected (or as it does in 2.6.* under the following condition:

this.route('test-route', { path: '/:routeId', resetNamespace: true }, function() {
  this.route('test-sub-route', { path: ':subRouteId' });
});

this.route('page-not-found', { path: '/*wildcard' });

Here's a Twiddle to better illustrate. Try entering /some-id as a path and the wildcard route takes priority. /some-id/test-sub-route works as expected. Seems to only be where there's a nested route on a route looking for an id.

(cc @nathanhammond and @rwjblue per our conversation at the SF ember meetup)

[rwjblue]

@derrickshowers - Thanks for reporting!

@bantic - Do you think you may have time to poke at this?

[bantic]

@rwjblue Yes, I can take a look tonight or tomorrow.

[nathanhammond]

@bantic @derrickshowers @rwjblue Confirmed to work correctly in ember-route-recognizer.

diff --git a/tests/acceptance/glob-weight-test.js b/tests/acceptance/glob-weight-test.js
new file mode 100644
index 0000000..3f27e45
--- /dev/null
+++ b/tests/acceptance/glob-weight-test.js
@@ -0,0 +1,13 @@
+import { test } from 'qunit';
+import moduleForAcceptance from '../../tests/helpers/module-for-acceptance';
+
+moduleForAcceptance('Acceptance | glob weight');
+
+test('Check glob weight.', function(assert) {
+  visit('/some-id');
+
+  andThen(function() {
+    assert.equal(currentURL(), '/some-id');
+    assert.equal(currentRouteName(), 'test-route.index');
+  });
+});
diff --git a/tests/dummy/app/router.js b/tests/dummy/app/router.js
index 9a57756..80c6896 100644
--- a/tests/dummy/app/router.js
+++ b/tests/dummy/app/router.js
@@ -8,6 +8,13 @@ const Router = Ember.Router.extend({
 Router.map(function() {
   this.route('foo');
   this.alias('alias', '/elsewhere', 'foo');
+
+  this.route('test-route', { path: '/:routeId', resetNamespace: true }, function() {
+    this.route('test-sub-route', { path: ':subRouteId' });
+  });
+
+  this.route('page-not-found', { path: '/*wildcard' });
+
 });

 export default Router;

In other words we have that as a fix-forward in our back pocket.

[bantic]

I'm not certain this is a route-recognizer bug. My best guess at the moment is that it's related to the interaction of ember's routing dsl and route-recognizer.

I ran the following test against the past several versions of route-recognizer (v0.2.0, v0.1.11, v0.1.10, 0.1.9 and 0.1.8), and it failed the same way in all of them:

test("support nested route and star route", function() {
  router.map(function(match) {
    match("/:routeId").to("routeId", function(match) {
      match("/").to("routeId.index");
      match("/:subRouteId").to("subRouteId");
    });
    match("/*wildcard").to("wildcard");
  });

  matchesRoute("/abc", [
    {handler: "routeId", params: { routeId: "abc" }, isDynamic: true},
    {handler: "routeId.index", params: {}, isDynamic: false},
  ]);
  matchesRoute("/abc/def", [
    {handler: "routeId", params: {routeId: "abc"}, isDynamic: true},
    {handler: "subRouteId", params: {"subRouteId": "def"}, isDynamic: true}
  ]);
  matchesRoute("/abc/def/ghi", [{handler: "wildcard", params: { wildcard: "abc/def/ghi"}, isDynamic: true}]);
});

The "/abc" path matches the wildcard handler (incorrectly), the "/abc/def" path matches correctly, and the "/abc/def/ghi" path fails to match anything. Test Output Screenshot.

I'll poke around some more later, hopefully tonight or tomorrow.

[bantic]

I'm not certain this is a route-recognizer bug.

To clarify: Not a new route-recognizer bug. And the fact that ember's routing has the issue in newer ember is probably related to some interaction between ember and RR rather than RR directly. I'll dig in further and update.

[rwjblue]

Sounds good. Thanks for digging in.

[bantic]

After looking more deeply, this is due to a bug in route-recognizer. I hadn't realized that Ember was using route-recognizer@0.1.5 in Ember@2.6 (see #13211).
After going back to 0.1.5 I was able to bisect the breaking change: tildeio/route-recognizer@0499446 which landed in route-recognizer@0.1.6.

I'm going to read the specificity/sorting rules (and tildeio/route-recognizer#46 and tildeio/route-recognizer#84) and work a bit on a fix for route-recognizer.
Ping @jmeas in case you have any thoughts here as well.

[jamesplease]

Thanks for the ping, @bantic. I'll look into this later today ✌️

[jamesplease]

This is surprising to me because dynamic segments have a specificity of 3 which should always win out against a wildcard's specificity of 1.

My hypothesis is that it's one of two things:

1. The wrong specificity is being compared. If it's not comparing 3 vs 1 here, then that would be considered "wrong." For this to occur, either the dynamic route needs to become less specific, or the glob needs to become more specific. I don't think there's anything in the code to make a route less specific, and I also don't think there's anything that could cause one route to increase the specificity of a separate route, so I think it's more likely that...
2. Other logic is superseding the specificity comparison. It'd be great if we could look at the specificity of the routes and determine what would match solely based on that, but the fix in Resolve issue where globs would be prioritized over empty segments tildeio/route-recognizer#84 demonstrates that that isn't always the case. Occasionally route recognizer will factor in other things that can cause less specific routes to get matched over more specific ones.

I should have time after work to step through this and figure it out if you don't get to it before me @bantic

Also very surprised that the tests don't cover this. @derrickshowers / @bantic if one of you could open a simplified failing test case in route recognizer that would help with the debugging later. Nbd if you don't have time tho'

---

Also, is ember-route-recognizer a rewrite of the route recognition algorithm? If so, that's awesome. I spent a lot of time looking at Ember's routing stuff, and I've always felt it could be implemented in a more straightforward way.

[bantic]

@jmeas Thanks for the description! Here's a failing PR: tildeio/route-recognizer#100

I got waylaid by a few other things but hope to have a chance to dig back in again today. I will update here if I start working on a fix.

[nathanhammond]

@jmeas Yes, ember-route-recognizer is a near ground-up rewrite of route-recognizer. It currently passes 100% of the tests, include the new one from #100. It has the added benefit of being an Ember addon and supporting serialization. Next steps are to improve the automated serialization, add support for engines, and performance tuning. It's landable once @stefanpenner and @rwjblue get us to Ember-as-an-addon.

Full documentation of the r-r research effort is here: http://www.nathanhammond.com/route-recognizer. Note that I wrote the specificity sections before finishing implementation and I need to revise them (doing that now).

[bantic]

@jmeas Thanks again for describing what may be going on. After a little digging with @mixonic just now, the specificity of added routes seems correct, so we think the case is:

Other logic is superseding the specificity comparison.

In particular, it may be related to adding the routes and compiling the NFA. During recognition of "/abc", e.g., the only matched state is the wildcard state.

And another thing that we noticed is that if we change the ordering of the route mapping in the test from:

  router.map(function(match) {
    match("/:routeId").to("routeId", function(match) {
      match("/").to("routeId.index");
      match("/:subRouteId").to("subRouteId");
    });
    match("/*wildcard").to("wildcard");  // wildcard last
  });

to

  router.map(function(match) {
    match("/*wildcard").to("wildcard"); // wildcard first
    match("/:routeId").to("routeId", function(match) {
      match("/").to("routeId.index");
      match("/:subRouteId").to("subRouteId");
    });
  });

then all the tests will pass.

I don't have any more time to dig in today, but may look at this over the weekend if I am able to.

[mixonic]

When adding a wildcard route, it appears the currentState.put code returns the same state as it was called on instead of always creating a new state. This causes the wildcard settings to basically stomp the previously exising segment settings.

For example the regex from the old dynamic segment is stomped when this line executes.

Still investigating. Seems pretty fatal though. I'm flummoxed as to how the commit @bantic identified caused this to surface.

[nathanhammond]

See also #13960.

[mixonic]

@derrickshowers I've published a build of Ember with a proposed fix. Can you please confirm this build fixes the issue for you in your real app?

• Updated twiddle with the fix: https://ember-twiddle.com/5508287a7dcd55f0764ba7c4e2c761f1?openFiles=router.js%2C
• The custom build of 2.7: https://s3.amazonaws.com/uni.madhatted.com/ember-2.7.0-routerfix/ember.debug.js Please note the whole Ember dist dir is there, so you can find min.js and prod.js etc.

[derrickshowers]

@mixonic Verified the build fixes the issue in my app and all tests pass. Thank you!