Bug in duplicate state name checking when using 'parent' definitions

[marklagendijk]

Currently there are 3 ways of defining states:

1. Using 'dot notation'. For example name: 'contacts.list'
2. Using the parent property with the parent name as string. For example: parent: 'contacts'
3. Using the parent property with the parent object. For example parent: contacts (where 'contacts' is an stateObject)

What you would expect is that method 2 and 3 would resolve the name to the 'dot notation'. However, currently this doesn't seem to happen. Because of this the code which checks for duplicate stateNames wrongfully throws errors when 2 children of different parents have the same name.
For example:

$stateProvider
    .state({
        name: 'contacts',
        templateUrl: 'contacts.html'
    })
    .state({
        name: 'list',
        parent: 'contacts',
        templateUrl: 'contacts.list.html'
    })
    .state({
        name: 'products',
        templateUrl: 'products.html'
    })
    .state({
        name: 'list',
        parent: 'products',
        templateUrl: 'products.list.html'
    });

Or object notation:

var contacts = {
    name: 'contacts',
    templateUrl: 'contacts.html'
};
var contactsList = {
    name: 'list',
    parent: contacts,
    templateUrl: 'contacts.list.html'
};
var products = {
    name: 'products',
    templateUrl: 'products.html'
};
var productsList = {
    name: 'list',
    parent: products,
    templateUrl: 'products.list.html'
};

$stateProvider
    .state(contacts)
    .state(contactsList)
    .state(products)
    .state(productsList)

Both throw an error:

Error: [$injector:modulerr] Failed to instantiate module `APP_MODULE` due to: State 'list'' is already defined

While defining this example using method 1 does work:

$stateProvider
    .state({
        name: 'contacts',
        templateUrl: 'contacts.html'
    })
    .state({
        name: 'contacts.list',
        templateUrl: 'contacts.list.html'
    })
    .state({
        name: 'products',
        templateUrl: 'products.html'
    })
    .state({
        name: 'products.list',
        templateUrl: 'products.list.html'
    });

[ksperling]

I don't consider this a bug... Your assumption that an explicit parent gets normalized into dot notation is incorrect. The opposite is true in that dot notation is used to infer parent if it is not explicitly provided, but this doesn't actually change the state name.

Having $state silently change the name could be rather confusing... as it stands the name you register is the same name you can use for $state.transitionTo, $state.is, or anywhere else a state is referred to by name.

[nateabele]

IMO this is a docs issue. @timkindberg It doesn't look like we have docs on this. Showing the different ways to define states/parents should probably be right on the main wiki page.

[timkindberg]

Oh it's always the docs guy's fault. I see how this works. jk.

I have a whole page dedicated to Nested States, granted its the second page in the guide, but I personally feel that first you need to learn how to make a single state and its properties before you move on to nesting multiple states.

What am I missing? Should I just add an intro to that page about the 3 explicit ways to create nested states?

[timkindberg]

So I added some stuff to the docs. Hopefully it helps clear this up a bit. https://github.com/angular-ui/ui-router/wiki/Nested-States-&-Nested-Views

[eahutchins]

I found this quite surprising, in effect for options 2 and 3 you need to name the states using dot notation to have equivalent navigation even though you aren't specifying the actual relationship that way? This is counter-intuitive and makes designing reusable components that much more of a pain (you have to catenate state names based on which parent state you attach to). This is extra confusing if state-relative addressing (in ui-sref) does work the way you expect for options 2 and 3. Is this set in stone at this point?

[timkindberg]

I don't think it's right either. We'll have to think of a solution.

[ksperling]

yeah, the relative addressing via sref and go() have obviously somewhat changed expectations...

I think it's important to have the ability to "move" sub-trees of the state tree (i.e. a reusable or semi-independent component) around in the tree by only changing the definition of the root of that sub-tree, without having to touch all the other states or changing every ui-sref etc.

[nateabele]

Sorry, I didn't fully digest this the first time around. Object states not resolving to fully-qualified names behind the scenes is obviously wrong.

I think it's important to have the ability to "move" sub-trees of the state tree (i.e. a reusable or semi-independent component) around in the tree by only changing the definition of the root of that sub-tree, without having to touch all the other states or changing every ui-sref etc.

Yeah, there's actually an issue there that I have to fix, so that ui-sref finds its closest anscestor ui-view to resolve relative states. After that, relative state targeting should always be based on whatever state the template is rendering in, which should resolve that aspect of the problem.

[ksperling]

@nateabele

Sorry, I didn't fully digest this the first time around. Object states not resolving to fully-qualified names behind the scenes is obviously wrong.

I disagree on this one... If define a state and tell $state that it is called "foobar", then I should be able to $state.transitionTo('foobar') and have a ui-sref="foobar" no matter what parent state I decide to hang "foobar" off.

[ksperling]

Maybe we can work out the 'relative' name of a state at registration time in addition to the absolute name... If the parent is inferred from dot-notation, the relative name becomes the part after the last dot.

Or another option would be to do it the other way around -- we DO derive an absolute name, but we still use the name as it is specified as an alias for the fully qualified name, as long as the alias is unique (and once it's no longer unique you just can't refer to the state via the alias anymore). Not ideal either though because it means stuff can break via changes at a distance.

[timkindberg]

Makes sense to me.

[nateabele]

@ksperling IMO state references should always be "fully-qualified" meaning that for object-defined states, use the object, and for strings, use the dot-separated path.

As you mentioned, uiSref changes the expectations somewhat (though I don't think this applies to go(), since it is fully capable of taking state objects). Here, I think the solution is to accept a relative or fully-qualified state name, the same as we would otherwise. We can normalize these behind-the-scenes, without impacting user-specified attributes.

[marklagendijk]

Good to see the discussion on this issue. I'm sure you guys will choose a good solution.

For whoever might find this interesting, currently I use the following helper functions for my state configuration:

function setState(state){
    fixStateName(state);
    $stateProvider.state(state);

    if(state.children && state.children.length){
        state.children.forEach(function(childState){
            childState.parent = state;
            setState(childState);
        });
    }
}

function fixStateName(state){
    var name = state.name;
    var currentState = state;
    while(currentState.parent !== undefined){
        currentState = currentState.parent;
        name = currentState.name + '.' + name;
    }
    state.name = name;
}

My application has one rootState. This rootState is a huge object which contains all the states of the application via children properties (each state can have an children array defined). What these functions do is:

1. Recursively set all the state + all its descendant states
2. Change the state names to the dot notation, to avoid name conflicts.

This allows for some powerful state related stuff. For example creating menus. MainMenu:

<li ng-repeat="state in rootState.children | filter:{ data.inMenu: true }"  ng-class="{ selected: $state.includes(state) }">

SecondaryMenu:

angular.module('MyApplication.controllers')
    .controller('SecondaryMenuController', ['$scope', '$rootScope', '$state', function($scope, $rootScope, $state){
        $scope.getSecondaryState = function(){
            for(var i = 0; i < $rootScope.rootState.children.length; i++){
                var secondaryState = $rootScope.rootState.children[i];
                if($state.includes(secondaryState)){
                    return secondaryState;
                }
            }
        };
    }]);

<li ng-repeat="state in getSecondaryState().children" ng-class="{ selected: $state.includes(state) }">

[eahutchins]

@ksperling what happens when I define a reusable component with a state foobar and attach it to multiple places in the state tree? IMO you should be able to do this and the name of the state should always be addressable via dot notation no matter which way the states are defined. Whether that changes the actual name of the state or not is a minor issue. If you really want to be able to address the leaves of the state tree by their simple/short name then I think you have to allow duplicates and handle looping through them forwards and backwards (an example use case would be to have help states which show user documentation, with navigation to next/prior help state possible via +help and -help or something similar). I'm not sure it's worth the complexity to support this though.

[jyboudreau]

+1 on fixing this. I think that using different state configuration options should not change the available navigation API. Especially since I find the first option for defining states names extremely tedious (apparent when you start having many nested states)