Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

$route property not available on next and current parameters in $routeChangeStart for default route #1907

Closed
maciejblinkbox opened this issue Jan 29, 2013 · 7 comments

Comments

@maciejblinkbox
Copy link

In $routeChangeStart event handler current and next parameters don't have the $route property when the route is default route.

I have put toghether a plnkr that shows the problem:
http://run.plnkr.co/nQePdzxAemAZ5Q2T/

Here are example routes:
app.config(function($routeProvider){
$routeProvider.when('/page/:id', {templateUrl: 'Main.html' , controller: 'MainCtrl'});
$routeProvider.otherwise({templateUrl: 'Main.html' , controller: 'MainCtrl'});
});

Expected behaviour: $route should be present on current and next parameters regardles whether a default route is hit or regular route.

@latentflip
Copy link
Contributor

@maciejblinkbox that plnkr link is saying "Not Found" for me, any chance of reposting it to a new one, or opening it up if you have it made private?

@maciejblinkbox
Copy link
Author

Appologies for that. This is the first time I used plunker.
I 'forked to public plunk' now, 'Show off your work' url is: http://plnkr.co/edit/kHfGG2
Does that work for you?
Thanks

@latentflip
Copy link
Contributor

I think this is an issue in your code. In your plnkr, you are trying to access next.$route and current.$route. But next and current are the routes themselves.

If you look at this updated plnkr: http://plnkr.co/edit/RjNJbC (preview here: http://embed.plnkr.co/RjNJbC/preview) you will see that everything works as you would expect.

There will be one time where current is not set and that is on the first routing on page load, as there is not current route being changed, only the next (first) one.

Does that make sense?

@maciejblinkbox
Copy link
Author

Interesting.

$route object on next and current objects has controller, reloadOnSearch and templateUrl properties.
next and current objects have params, pathParams, scope and $route.
So they are not the same.

In the plunker the app opens default route on the initial load.
In $routeChangeStart I get:
current - undefined - correct
next - defined (default route) and doesn't have $route which I believe is incorrect.

When I go to any other route, let's say I click on the "Page 1" link in $routeChangeStart I get:
current - defined (default route) and doesn't have $route which I believe is incorrect.
next - defined (page route) and it does have the $route property which is correct

So current and next objects in $routeChangeStart:

  • do have the $route property if we are on a regular route
  • don't have the $route property if we are on a default route

This behaviour is inconsistent.
Expected behaviour: $route property is always present.

@latentflip
Copy link
Contributor

Okay, apologies, my bad. I see what you are saying now.

Yes this does indeed look to be a bug. Although as far as I can tell there are no tests on the $routeProvider that check if $route will be set ever.

This seems to boil down to this line: https://github.com/angular/angular.js/blob/master/src/ng/route.js#L469 $route is only set when a route is matched, not when it falls back to the default route.

Changing the parseRoute function to something like this fixes the issue, but since there are no tests at all for the $route stuff they will probably have to be written before this can get merged in.

@maciejblinkbox feel free to turn this code into a tested pull request if you like :)

    /**
     * @returns the current active route, by matching it against the URL
     */
    function parseRoute() {
      // Match a route
      var params, match;
      forEach(routes, function(route, path) {
        if (!match && (params = switchRouteMatcher($location.path(), path))) {
          match = inherit(route, {
            params: extend({}, $location.search(), params),
            pathParams: params});
          match.$route = route;
        }
      });
      // No route matched; fallback to "otherwise" route
      if (!match && routes[null]) {
        match = inherit(routes[null], {params: {}, pathParams:{}});
        match.$route = routes[null];
      }
      return match;
    }

maciejblinkbox added a commit to maciejblinkbox/angular.js that referenced this issue Jan 31, 2013
…t parameters in $routeChangeStart for default route
maciejblinkbox added a commit to maciejblinkbox/angular.js that referenced this issue Jan 31, 2013
… parameters in $routeChangeStart for default route
@IgorMinar
Copy link
Contributor

nextRoute.$route is not meant to be public api use nextRoute instead directly which inherits from the currently matched route definition.

I added tests just to be sure that we do the right thing and renamed nextRoute.$route to nextRoute.$$route to make it clear that this is a private property.

@IgorMinar
Copy link
Contributor

we should reimplement the matching so that event $$route is not exposed. when we do it we'll break you. don't depend on it. there shouldn't be a reason for you to do so.

IgorMinar added a commit that referenced this issue Mar 8, 2013
the `nextRoute` object available in `$routeChangeStart` handler
accidentaly leaked  property which pointed to the route definition
currently being matched.

this was done just for the internal needs of the `$route` implementation
and was never documented as public api.

Some confusion arouse around why the $route property was not always
available on the `nextRoute` object (see #1907). The right thing for us
to do is to prefix the property with $$ for now and refactor the code
to remove the property completely in the future. Application developers
should use the `nextRoute` object itself rather than its `$route` property.
The main diff is that nextRoute inherits from the object referenced by $route.

BREAKING CHANGE: in $routeChangeStart event, nextRoute.$route property is gone.

Use the nextRoute object instead of nextRoute.$route.

Closes #1907
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants