angular-ui / ui-router

The de-facto solution to flexible routing with nested views in AngularJS
http://ui-router.github.io/
MIT License
13.54k stars 3k forks source link

Due to fix in 0.2.13 .when('/parent', '/parent/child') is not working with ui-sref #1584

Closed Radim-Kohler closed 9 years ago

Radim-Kohler commented 9 years ago

I would say, that the _"fix"_ in 0.2.13, related to: Avoid re-synchronizing from url after .transitionTo, change: 19715d15 introduced a new issue. Or at least, the .when('/parent', '/parent/child') is not working for ui-sref="parent"

Description of the "issue"

As documented here: How to: Set up a default/index child state small extract:

Or if you want the 'parent.index' url to be non-empty, then set up a redirect in your module.config using $urlRouterProvider:

$urlRouterProvider.when('/parent', '/parent/index');
$stateProvider
    .state('parent', {url: '/parent', abstract: true, template: '<ui-view/>'} )
    // url '/parent/index'
    .state('parent.index', {url: '/index'} )

NOTE: in the above cite, I replaced the home in url with parent to make it more consistent

This has stopped working. There are plunkers:

Examples

There are examples of the issue with different versions:

this is the change applied in "roll back" of a fix 19715d15 0.2.13 inside of the released angular-ui-router.js

1988   ...
1989   // RKö - rollback of a fix
1990   // if (ignoreUpdate) return true;
1991    ...

Workaround without change of 0.2.13

I described how to go around here Angular UI-Router $urlRouterProvider .when not working when I click <a ui-sref=“…”>, we need eventing:

var onChangeConfig = ['$rootScope', '$state',
 function ($rootScope, $state) {

  $rootScope.$on('$stateChangeStart', function (event, toState) {    
    if (toState.name === "parent") { 
      event.preventDefault();
      $state.go('parent.index');
    }
  });

}]

Maybe this is the way to go (or please, give me better). But as mentioned below, not sure about consistency...

Question, is it an issue? or not

I can imagine that this issue would be refuesd as proper behaviour.

BUT, is it really what we want - if this will result in 2 different states:

<a ui-sref="parent">  // goes to parent
<a href="#/parent">    // goes to parent.index
dmitriynosenko commented 9 years ago

+1, I have the same issue.

J22Melody commented 9 years ago

+2

see http://stackoverflow.com/questions/27120308/angular-ui-router-urlrouterprovider-when-not-working-when-i-click-a-ui-sref

ashjmcfox commented 9 years ago

+1

As mentioned, this prevents redirecting URLs/states when using ui-sref. This is unfortunate as it now means I cannot upgrade past 0.2.12 since I require this functionality. I don't think we should have to clutter our $stateChangeStart event just to handle this, especially if we have multiple URL/state redirects in place. Hopefully this issue can be fixed in 0.2.14.

adestis-ds commented 9 years ago

+2 I have a "real world" example showing the same issue:

http://plnkr.co/edit/xyVPZePL2eV0lp8HxPBG?p=preview

it works with 0.2.12 but nOT with 0.2.13. If the new behaviour is intended I would like to know what is the recommended way for setting default states.

christopherthielen commented 9 years ago

People who are using .when this way: do you really want "state-based redirection"? .when is for matching a url and redirecting, and/or doing some other action. If this issue affects you, it means you are navigating using states, not url. I think what you are doing is navigating to a state (using state.go/ui-sref), but you really want to navigate to some other state (usually a substate).

In UI-Router the state is the primary focus, and the URL that maps to that state is secondary. In my eyes a state-based redirect is preferable to a url based one. Url redirection is more very loosely coupled and feels sloppy when compared to state-based redirection.

Read my analysis of the "double transition" problem here: #1573. As noted in the issue, avoiding re-sync is a safety net to avoid double-transitions. However, the primary fix is to ensure that all state-to-url mappings are bi-directional.

As such, since state based redirection using $stateChangeStart in 0.2.x is rather clumsy, I'm going to comment out the "avoid resync" logic for 0.2.14. I'm going to reintroduce it in 1.0 when we have a better state-level redirection API. If there are any other use cases that aren't "state-level redirection", please inform me so I make sure to account for them.

rguldener commented 9 years ago

+1 same problem

As for the use case: Better (i.e. non-central) state forwarding would be very welcome, also using the urlRouterProvider based forward for what really should be a state forward. The parent state is just a placeholder for us (which when called should redirect to a default child state) and we reference it in ui-sref to get "automagically" correct setting of the active class in the navigation. Since we have this use case in many places of our app we do not want to clutter the $stateChangeStart function with state redirects and thus opted to use $urlRouterProvider.

RobiFerentz commented 9 years ago

:+1:

Same issue!

yaru22 commented 9 years ago

If this issue affects you, it means you are navigating using states, not url. I think what you are doing is navigating to a state (using state.go/ui-sref), but you really want to navigate to some other state (usually a substate). @christopherthielen

For me, the reason I navigate to a parent state rather than its substate directly is because of ui-sref-active.

For example, I have a side menu item associated with, let's say, app.settings state that needs to be highlighted (with .active class) when I go to any of its substates (e.g. app.settings.a, app.settings.b, app.settings.c). app.settings state is just a common state encompassing those substates. So my html looks like:

<div ui-sref-active>
  <a ui-sref="app.settings">Settings</a>
</div>

When I click on Settings link, I want it to redirect me to the first substate (i.e. app.settings.a ) automatically. If I put app.settings.a directly in ui-sref above, Settings menu item will only be highlighted when the app is in app.settings.a but not when it's in app.settings.b or app.settings.c.

Maybe there is a better way of using ui-sref-active that I'm not aware of? Another workaround is using ng-class with $state.includes(), but I was hoping to make it work with ui-sref/ui-sref-active alone.

So if I want to keep <a ui-sref="app.settings">Settings</a> and have it redirect me to app.settings.a, I need $urlRouterProvider.when() work as before.

If there is a better way to handle the case above, I'd like to know as well. Thanks.

fubupc commented 9 years ago

@yaru22 +1

Exactly the same scenario here!

jeverden commented 9 years ago

+1 Same issue

squadwuschel commented 9 years ago

+1

corbanbrook commented 9 years ago

+1

lazychino commented 9 years ago

:+1: I have the same issue, I use it like @yaru22 for ui-sref-active and for defining the default subroute with ui-router-extras deep state redirect

Zacharias3690 commented 9 years ago

+1

yahyaKacem commented 9 years ago

@christopherthielen another use case besides the one presented by @yaru22 3 main states home, profile and settings like this: qq

The 2nd state profile have 2 nested states profile.about and profile.main qqq

I need that the profile state clickable on the index page loads the profile view and redirects to the default nested state which is profile.about here qqqq

Here's a working plunker with what I got so far, with the fix mentioned by @Radim-Kohler.

ehacke commented 9 years ago

:+1:

acollard commented 9 years ago

+1

ui-michal-szwed commented 9 years ago

:+1:

goddyZhao commented 9 years ago

+1

summer-liu commented 9 years ago

+1

gabrielmaldi commented 9 years ago

In my case the underlying issue is in fact a feature request: we need a way to specify default child states.

Given:

$stateProvider
    .state("settings", {
        abstract: true,
        url: "/settings"
    })
        .state("settings.general", {
            url: "/general"
        })
        .state("settings.notifications", {
            url: "/notifications"
        })
        .state("settings.advanced", {
            url: "/advanced"
        })

It would be nice if we could say something like:

.state("settings.general", {
    url: "/general",
    isDefaultChild: true
})

So that when we navigate to the abstract parent state:

$state.go("settings")

We would get redirected (or even better: go directly) to settings.general.

I used to do this using when but it would certainly be nicer to have a "isDefaultChild" property on each state.

What do you think?

Thanks

yahyaKacem commented 9 years ago

Also one more thing to be aware of when using the fix mentioned by @Radim-Kohler is that the order of defining the data object in the state change so when using this solution the child state's data gets defined before the parent state's data so the parent overwrite the child's data (a more detailed explanation could be found in this bug report) so if you'r using something that require you to pass data to the child state with the data attribute in the state definition object you should be aware of this bug, for me I think the best solution here is to define all the data for children in the parent state until this gets fixed.
@gabrielmaldi that's correct this is more of a feature request then a bug, and +1 for your suggestion also other possibility is to put the default child name in the parent so instead of having isDefaultChild: true on a child state we'll have a defaultChild: "childname" on the parent state, I think both are valid solutions.

cs-NET commented 9 years ago

@yahyaKacem I agree that @gabrielmaldi suggestion should be in the parent state and would like to expand on that idea by adding another option childSticky: true so that when navigation occurs to another parent the state provider will keep the previously active child as the default instead of resetting back to the childDefault: "childName".

gabrielmaldi commented 9 years ago

I didn't really give the proposed syntax a lot of thought, just wanted to outline the idea of default child states. Perhaps it would be better to have it in the parent state, or maybe someone comes up with a better way.

@cs-NET I think you should look into UI-Router Extras which has Sticky States and Deep State Redirect.

violet-day commented 9 years ago

+1

killbirds commented 9 years ago

+1

getvega commented 9 years ago

@yahyaKacem like these redirections defined on the parent - much cleaner than have it outside à-la-urlRouterProvider. One problem though: state parameters. What if you had something like:

$stateProvider
    .state("posts", {
        url: "/posts",
        defaultChild: "posts.post"
    })
        .state("posts.post", {
            url: "/:id"
        })

Which id would we inject in posts.post?

That defaultChild would have to also specify which params, and maybe be an invokable similar to controllers :

$stateProvider
    .state("posts", {
        url: "/posts",
        defaultChild: "posts.post",
        defaultChildParams: ["posts", function(posts) {
                return {
                        id: posts[0].id
                };
        }]
    })

Not so clean anymore... What do you think?

yahyaKacem commented 9 years ago

@getvega I'm actually doing that differently, I just inject the posts service into the run function then get the posts[0].id from there and form the parameters object in the run function then when I call $state.go I pass the params there:

var onChangeConfig = ['$rootScope', '$state', 'posts', function ($rootScope, $state, posts) {
   $rootScope.$on('$stateChangeStart', function (event, toState) {
     if (toState.name === "posts") {
       event.preventDefault();
       $state.go('posts.post', {id: posts[0].id});
    }
  });
}];

as for the defaultChildParams as you said not so clean maybe the defaultChild should be a string with child name in case no route params are needed and in case of params, object like {child: 'childName', params: {id: 0}}, where params could be a simple variable or a function that takes some injectables and returns the params object.

acollard commented 9 years ago

@yahyaKacem This may make your code a little more reusable. This is how we handle redirects.

.config(['$stateProvider', '$urlRouterProvider',function($stateProvider, $urlRouterProvider) {
    $stateProvider
        .state('Account.Security', {
          url: '/security',
          redirectTo: 'Account.Security.ChangePassword' // OR
          //redirectTo: function($state){ $state.go('Account.Security.ChangePassword', { id: 'some id' }); }
        });
});

.run(['$rootScope', '$state', function($rootScope, $state){
      $rootScope.$on('$stateChangeStart', function(event, toState) {
        var redirect = toState.redirectTo;
        if (redirect) {
          event.preventDefault();
          if(angular.isFunction(redirect))
              redirect.call($state);
          else
              $state.go(redirect);
        }
      });
    }]);

The redirectTo function could do whatever logic you needed. Ex. get the id for the last item the user was viewing.

gabrielmaldi commented 9 years ago

@getvega perhaps the redirection should just forward any parameters it got (e.g. from $state.go()) to the child state; that'd reduce the complexity.

christopherthielen commented 9 years ago

@gabrielmaldi @acollard @yahyaKacem @getvega Great conversation. Let's move the discussion to #1235 to keep this one about url-based redirects

christopherthielen commented 9 years ago

For 0.2.13, here is a very simple recipe you can use to redirect to a default substate:

  $stateProvider.state('profile' , {
      url: "/about",
      templateUrl: "profile.html",
      redirectTo: 'profile.about'
  });

 app.run($rootScope, $state) {
    $rootScope.$on('$stateChangeStart', function(evt, to, params) {
      if (to.redirectTo) {
        evt.preventDefault();
        $state.go(to.redirectTo, params)
      }
    });
  }

See it in action in a fork of the above plunkr that @yahyaKacem posted:

http://plnkr.co/edit/dcN7Qra6nY9ZTOGjgCvJ?p=preview

VictorioBerra commented 9 years ago

@acollard

How can I gain access to resolved items in my redirectTo function? It would make sense that id need to resolve an item from the database and the state of that item could effect what params or states I need to redirect to.

http://plnkr.co/edit/6opdJIQe09ULqZxiGZOZ?p=preview

acollard commented 9 years ago

@LordWingZero

Accessing parent resolves is a little difficult, the process of running the resolves happens inside the $state.transitionTo and are not accessible through the event. Accessing the toStates resolves will likely not be possible at all, since redirectTo is processed before the toStates resolves are run.

But if you can use a registered service instead of a parent resolve then below will work for you.

We've changed our method above to support dependency injection. My code above has been changed to:

module.run(['$rootScope', '$state', '$injector', function ($rootScope, $state, $injector) {
    $rootScope.$on('$stateChangeStart',function (event, toState, toParams) {
      var redirect = toState.redirectTo;
      if (redirect) {
        if (angular.isString(redirect)) {
          event.preventDefault();
          $state.go(redirect, toParams);
        }
        else {
          var newState = $injector.invoke(redirect, null, { toState: toState, toParams: toParams });
          if (newState) {
            if (angular.isString(newState)) {
              event.preventDefault();
              $state.go(newState);
            }
            else if (newState.state) {
              event.preventDefault();
              $state.go(newState.state, newState.params);
            }
          }
        }
      }
    });

Here is a sample of using it:

.config(['$stateProvider', '$urlRouterProvider',function($stateProvider, $urlRouterProvider) {
    $stateProvider
        .state('Account.Security', {
          url: '/security',
          redirectTo: ['auth', function(auth){ return auth.isLicensed() ? { state: 'Account.Security.ChangePassword', params: 'some id' } : 'AccessDenied';}]
        });
});

The ui-router team is looking to add this functionality natively #1235. My guess is parent resolves will still be unavailable. RedirectTo will likely occur before any resolves are run. Thoughts @christopherthielen?

VictorioBerra commented 9 years ago

@acollard

How would you handle redirectTo if auth.isLicensed() returned a promise instead of a bool?

http://plnkr.co/edit/6opdJIQe09ULqZxiGZOZ?p=preview

acollard commented 9 years ago

@LordWingZero

I'm assuming the promise from your redirectTo would still return one of the two options: the state name, or { state, param } object.

I would probably change the listener to this to support promises:

module.run(['$rootScope', '$state', '$injector', '$q', function($rootScope, $state, $injector, $q) {
        $rootScope.$on('$stateChangeStart', function(event, toState, toParams) {
          var redirect = toState.redirectTo;
          if (redirect) {
            if (angular.isString(redirect)) {
              event.preventDefault();
              $state.go(redirect, toParams);
            } else {
              var newState = $injector.invoke(redirect, null, {
                toState: toState,
                toParams: toParams
              });
              if (newState) {
                event.preventDefault();
                $q.when(newState).then(function(result) {
                  if (angular.isString(result)) {
                    $state.go(result);
                  } else if (result.state) {
                    $state.go(result.state, result.params);
                  }
                });
              }
            }
          }
        });

RedirectTo would look like this:

redirectTo: ['auth', function(auth){ return auth.isLicensed().then(function(isLoggedIn){ return isLoggedIn ? { state: 'Account.Security.ChangePassword', params: 'some id' } : 'AccessDenied';});}]

Alternatively you could use a resolve to do the redirect. We do this as well, which already supports promises and injecting resolves (parents and self).

$stateProvider.state({ 
    name: 'home', 
    url: '/home', 
    resolve: {
      loginRequired: ['$q','$state','auth', function($q, $state, auth){
       return auth.isLoggedIn()
        .then(function(isLoggedIn){
         if(!isLoggedIn) 
          {
             $state.go('login'); 
        }
       }
      }]
    }});
VictorioBerra commented 9 years ago

@acollard

What do you think of:

else if(angular.isFunction(newState.then)){
    event.preventDefault();
    newState.then(function(newStateData){
        $state.go(newStateData.state);
     });
}

That way it handles $promise objects (if using $resource you will need to return $promise probably. not sure) It handles strings, and it handles objects with a .state property.

acollard commented 9 years ago

@LordWingZero

My option above supports promises, strings, and objects. $q.when() is used to wrap all three options and return a promise. If a non-promise is wrapped then it will run then() immediately.

Checking for a promise yourself may not work well, I find it is easier to leave it up to angular.

gwin003 commented 9 years ago

Has this been resolved yet? I implemented @christopherthielen 's solution for now (thanks for that!), but it would be great if this got fixed.

VictorioBerra commented 9 years ago

@gwin003 it will be fixed with the next MAJOR release of ui-router. Almost everything will be different...

gwin003 commented 9 years ago

Thanks @LordWingZero

franklin-ross commented 9 years ago

@acollard

I'm liking your solution, but I added some code to support object literals like redirectTo: { state: "a.b", params: { id: 0 } }. Don't think it supported that case before?

        //Redirect to string
        } else if (redirect.state) {
          event.preventDefault();
          $state.go(redirect.state, redirect.params);
        } else {
        //Redirect to injectable
christopherthielen commented 9 years ago

This missed 0.2.14, sorry. I'll get this into 0.2.15 ASAP

christopherthielen commented 9 years ago

UI-Router 1.0 preview is available: https://github.com/angular-ui/ui-router/tree/feature-1.0

Radim-Kohler commented 9 years ago

I admire that... you are magicians. I have to touch it, start to test... upgrade... The idea about migrating to Typescript is MUST. We are on Typescript for 1,5 year... cannot imagine JS development without it anymore. Looking forward experience with the mighty 1.0 ;)