[bug] Child state with stateParams will load controller twice if stateParam absent #125

carlatposse · 2013-05-08T02:47:01Z

Hard to explain, so I've whipped up a minimal repro

Go to index.html#/bar
Click the baz button and watch the console.log. It will print out the scope twice. Whats worse is one of these scopes is orphaned after the digest and loses its $parent.

If the baz 1 is clicked, the id route param is supplied and the bug is not occur.

Also, would it be worth defaulting toParams in transitionTo(to, toParams) to {} so the user does not need to specify the empty object for when params should be empty.

I tried finding the bug myself but I couldn't find it and don't have the time to investigate further.

Thanks :)

<!doctype html>
<html lang="en" ng-app="repro">
  <head>
    <script src="angular-1.1.4.js"></script>
    <script src="angular-ui-states.js"></script>
  </head>
  <body>
    <div ui-view></div>
  </body>
  <script>
  angular.module('repro', ['ui.state'])
    .config(['$stateProvider', function ($stateProvider) {
        $stateProvider
          .state('foo', {
            url: '',
            template: 'foo <div ui-view><' + '/div>',
            controller: ['$scope', function($scope) { $scope.name = 'foo'; }]
          })
          .state('foo.baz', {
            url: '/baz/:id',
            template: 'baz',
            controller: ['$scope', function($scope) {
              $scope.name = 'baz';
              console.log('scope', $scope);
            }]
          })
          .state('foo.bar', {
            url: '/bar',
            template: 'bar -> <button ng-click="a()">baz<' + '/button><br>bar -> <button ng-click="b()">baz 1<' + '/button>',
            controller: ['$scope', '$state', function($scope, $state) {
              $scope.name = 'bar';
              $scope.a = function() {
                $state.transitionTo('foo.baz', {});
              };
              $scope.b = function() {
                $state.transitionTo('foo.baz', {id: 1});
              };
            }]
          });
    }]);
  </script>
</html>

The text was updated successfully, but these errors were encountered:

ksperling · 2013-05-11T00:56:36Z

The problem is that the 'id' parameter is required, but since you're not providing it it gets set to null inside transitionTo(). When $state then updates $location, the URL ends up as /baz/, which then triggers $state again to see what state that location maps to; in this case it maps to foo.baz with id='', which is different from the current state (because '' != null) so another transition is performed.

The bug is that transitionTo() should validate that all parameters have been specified, even though currently $state doesn't know if a particular parameter is optional or not. The other option would be treating '' and null the same, or maybe normalizing one to the other, but none of those seems ideal.

ksperling · 2013-05-24T15:31:53Z

@nateabele what are your thoughts on null vs empty string in this context to work around these types of issues?

nateabele · 2013-05-24T16:02:39Z

The simplest thing would be to treat null and "" as equivalent, but this could lead to unexpected behavior. I actually think coercing empty strings to null makes sense though.

Again, the URL is an imperfect representation of application state. I think it's better to explicitly acknowledge this, and let the state manager worry about the details of going from URL to state and vice-versa.

The other idea that this suggests is explicit parameter typing, i.e. '/baz/{id:integer}', which would not only allow regex matching internally, but would also provide a useful way of coercing the values in $stateParams before they are injected.

nateabele · 2013-05-24T16:05:11Z

Forgot to conclude: this opens the possibility of having '/baz/{id:string}', where $state can ensure that id is always coerced to an empty string, even if null.

paullryan · 2013-05-24T16:11:37Z

@nateabele I really like this suggestion as the type safety added would allow for some interesting defaulting patterns. Speaking of defaults do you see the default values here as configurable on initialization of the state, the stateProvider or none of the above.

nateabele · 2013-05-24T16:13:34Z

IMO the best way would be to allow params to be an object hash, and to be present event when url is also specified. @ksperling Thoughts?

timkindberg · 2013-05-24T16:32:16Z

It is odd that you can only specify params when a URL is also specified.
What if someone doesn't want to use urls but still have params?

paullryan · 2013-05-24T17:38:52Z

So something like the following?

.state('mystate', {
  params: [
    {param1: {default: 'xyz', type: 'integer'}},
    {param2: {type: 'string'}}
  ]
})

or (with url)

.state('mystate', {
  url : '/path/to/{param1:integer}/with/{param2:string}',
  params: [
    {param1: {default: 'xyz'}
  ]
})

with both of these resulting in the same $stateParams.

nateabele · 2013-05-24T17:41:08Z

Unless we're planning to allow params to be decorated with some information other than a default value, I think params: { key: "value" } should work fine. Especially since default is a reserved word. ;-)

paullryan · 2013-05-24T17:42:01Z

Doh, and great thought :)

ksperling · 2013-05-24T22:26:56Z

You can specify state.params even when there is no URL, but currently it's just an array of param names.

I think it should look something like this:

  params: {
    id: {
      type: 'string',
      optional: true,
      dynamic: true, // don't transition on change, instead two-way bind between $stateParams and $location.replace()
      encode: function (value) {
        // this refers to the parameter meta-data object
        return encodeURIComponent(String(value));
      },
      decode: function (string) {
        // inverse of encode
      }
    }
  }

I think most of the responsibility for this stuff should be with UrlMatcher; the optional parameters map would be passed into the UrlMatcherFactory where suitable defaults and normalization would be applied. In simple cases UrlMatcher would infer most options from the string pattern. $state can access the meta-data via UrlMatcher.parameters() as it does now, to get at flags like 'optional' or 'dynamic' (dynamic being an example of parameter meta-data that UrlMatcher itself is not interested in and just passing through unmodified).

One feature that is somewhat difficult to cover is wildcard query parameter support -- i.e. the ability to have all query parameters show up in $stateParams. The ability to explicitly enumerate all valid parameters is very useful, and without it deciding changes in which parameters apply to which state is difficult. Maybe UrlMatcher could support a special syntax like "...?" to enable wild card parameter support, but map this to a single parameter with custom encode/decode so that all query parameter get decoded into a single state parameter called '' that happens to be a key-value map. So $state itself would see it as one parameter. This also solves the problem of wildcard search parameters trampling over the names of path parameters. The only added complexity for $state would be to support equality tests between complex parameter values (which could be delegated to a param.equals() function).

One thing I'm not sure is a good idea is default values -- if you've got "/place/:name" and name defaults to 'home', wouldn't that imply that navigating to "/place/" would have to do redirect to "/place/home"? I suppose this might not be too hard to arrange. In case any defaulted parameters are empty, UrlMatcher.exec() would (optionally) return a redirect string instead of the parameters map, and $urlRouter would simply apply the redirect instead of calling through to the handler -- or some other similar mechanism.

nateabele · 2013-05-24T23:36:26Z

You can specify state.params even when there is no URL, but currently it's just an array of param names.

Yeah, I meant you can't specify both, because currently it throws an error.

I think it should look something like this [...]

The encode/decode idea is freaking awesome. It allows for exactly the kind of high-level abstraction that this project should provide. Way to take it to the next level!

I see a few things that I think could be improved though:

Instead of dynamic, you could be to have reloadOn parameter at the state level, because then you could do reloadOn: ["foo", "bar"], or just reloadOn: false, rather than having to re-type dynamic: true over and over again... also, it sounds like this is information that UrlMatcher would just end up communicating straight back to $state anyway
Parameters don't need an optional flag, since whether a parameter is optional or not can be inferred from the presence or absence of a default value, the lowest common denominator default value being null
The encode/decode functions could be wrapped in a top-level key, i.e. handler, which would promote composable and reusable "parameter objects" (think of a generic query/serialize interface to $resource objects)
Finally, it looks like your proposal includes everything... except a default value. ;-) How were you planning to address that? Keep in mind, use of the word default would mean having to quote it every single time, which IMO would be tedious.

One feature that is somewhat difficult to cover is wildcard query parameter support

From my experience of writing and rewriting URL routers for major web frameworks at least 4 times over the past ~6 years, mixing optional vs. required parameters with wildcard parameters coming from different places is extremely dangerous territory. You end up trying to hack around logical inconsistencies that are impossible to resolve.

I definitely concur with your idea of keeping query parameters separate, though maybe with a legal identifier like $query.

One thing I'm not sure is a good idea is default values

Trust me, they're super-useful, and simplify a bunch of things (some of which are listed above). :-)

[I]f you've got "/place/:name" and name defaults to 'home', wouldn't that imply that navigating to "/place/" would have to do redirect to "/place/home"

Nope, that would mean /place and /place/home are the same logical state. Going from one to the other would have no effect, and no redirects would be required. The matching state would just be invoked with $stateParams being populated with the default parameter values.

This is sort of the basis for how route design is done server-side, and it should map pretty easily to state trees. Let me know if you need a more in-depth explanation.

ksperling · 2013-05-25T21:10:46Z

Hm yeah combining encode/decode is a good idea. Why don't we just call it the "type" of the parameter, and have the encode and decode methods on the type object, as well as any other ones we might need (equals() comes to mind for comparing complex values). The type could also provide a regex pattern so that it doesn't need to be repeated each time you use e.g. an 'int' parameter (obviously the concrete pattern can still override).

I think the problem with handling default without a redirect is that the URL will change anyway as soon as the change of a dynamic parameter causes an update to the location. E.g. if you had '/places/{name}?orderby' with orderby being dynamic, changing orderby would cause the state to be serialized again and you'd end up changing it from '/places' to '/places/home?orderby=whatever'. It's not a huge problem obviously, but I think it's nicer if the URL reflects the canonical representation of the state (which is also good practice when doing it server-side in terms of SEO etc).

We could extend the curly-brace syntax for UrlMatcher so that a type name can be specified instead of a regexp (they should be easy to distinguish by restricting type names to identifiers), and allow them to be registered via $urlMatcherFactory.registerType() or some such method. Possibly even default values (as long as they don't contain special characters). Could end up with something like

url: '/place/{name=home:placeType}?{~orderby=name}' // '~' -> dynamic parameter

I don't really like having 'reloadOn' as a separate array, because I think to reload on a parameter should be the default behaviour as it is now, and just having the ability to specify a params object that gets merged with whatever options we parse out of the URL pattern is more generic. I think 'dynamic' would also imply a more sophisticated behaviour than simply not reloading, namely the two-way binding to stateParams. So the above would be equivalent to

url: '/place/:name?orderby'
params: {
  name: {
    type: 'placeType',
    def: 'home'
  },
  orderby: {
    def: 'name',
    dynamic: true
  }
}

nateabele · 2013-05-25T23:07:55Z

Why don't we just call it the "type" of the parameter, and have the encode and decode methods on the type object

That works.

I think the problem with handling default without a redirect is that the URL will change anyway as soon as the change of a dynamic parameter causes an update to the location.

Okay I see what you mean. That's fine with me. I think for URL parameters the default values can be inserted in the URL, but for querystring parameters, if the value matches the default, it should just be omitted.

Could end up with something like [...]

I'm wary of making the syntax overly complex. I think if we just allow /{name:type} in the URL, and params: { name: "value" } as a short-hand for params: { name: { def: "value" } }, that should be enough. I guess ~ for dynamic seems okay though.

toddhgardner · 2013-09-17T21:29:16Z

Hey Guys! I'm going to take a run at the type encoding part of this implementation and send in a pull request. I plan to implement the params portion of the API as follows (derived from your conversation):

url: '/myrouter/:id?startOn'

params: [
  "id": {
    // supports integer, boolean
    type: "integer",

    def: 0
  },
  "startOn": {
    type: {
      // evaluated for equality.
      equals: function (otherObj) {},

      // returns complex type from $location string
      decode: function () {}, 

      // returns string representation of complex type to be put in $location
      encode: function () {},

      def: new Date()
    }
  }
]

Here is how I believe I should implement this, but I would appreciate your thoughts:

remove check that prevents url and params from being specified
check and split types on urlMatcher
compile type in urlMatcher (contains the defaults for integer and boolean typing). Type is left attached to the urlMatcher object.
urlMatcher.exec should use the type.decode function and return the decoded type.
urlMatcher.format should use the type.encode function and return the encoded type.

Thanks!!

thebigredgeek · 2013-09-18T06:11:43Z

I've already been able to implement a work around for this, but it would be great if we could embed it into the actual router.

I used this:

http://mjtemplate.org/examples/rison.html

I wrapped it in a service, then created a wrapper service that injected $state and $rison (see above). This allowed me to treat stateful parameters as a hash table, with the following functions:

$stateManager.state([state]) //gets the state, OR sets the focus state.  The old state is still running, but the focus 
                                          //state's parameters can be manipulated and if a flush occurs BEFORE the old state
                                          //is made focus again, the app moves in entirety to the new state

$stateManager.set(key,val) //sets a key/val pair within the focus state's parameter
$stateManager.get(key) //get a val from a key within the focus state's parameter
$stateManager.del(key) //obvious
$stateManager.hset(hash,key,val) //obvious
$stateManager.hget(hash,key) //obvious 
$stateManager.hdel(hash,[key]) //obvious
$stateManager.flush() //pushes changes to the actual URL and load whatever state is currently in focus, as set 
                                 //above

I chose a hash table because I felt as though there wasn't any real use case that it couldn't handle. However, you can really encode infinitely deep objects using the default get, set, and del, the hash functionality is simply there for convenience to access 1st level object members.

I suggest that with UI-Router, we allow the developer to pass in a plain javascript object as a parameter that gets parsed using some sort of syntax (like Rison), and when fetched is parsed back into object state. If it isn't an object, it just works as it always has and thus, no regression.

However, what I think would be a real win in terms of functionality would be the ability to inject URL parameters with a function (lets call it ".alter(state,params,options)" ) that manipulates the URL without actually RELOADING the state (if oldState == newState, do nothing!, but ONLY for this method). Then, emit some sort of event on $rootScope to allow controllers to handle the state parameters change in their own way. This is ABSOLUTELY ESSENTIAL for any sort of breadcrumbs functionality on large-scale apps (like the one I am spearheading at work, and USING THIS MODULE), because the user needs to be able to reproduce a given state to the Nth degree of detail when copying and pasting the URL to a friend or bookmarking (limitless encoding, and sharing ALL reproduce-able stateful data with the state parameters so that the precise user experience can be reproduced seamlessly with a URL).

This would need to work vertically as well, allowing the parameters of parent states to be altered by children, and the parameters of children to be altered by parents (the tricky part). A way that I would suggest to accomplish this would be to provide a separate namespace within the $state service that allows the developer to change to a particular state as 'focused' (as described above), and allows the developer to manipulate stateful parameters and flush these changes to the URL.

This extends the API a bit on the $state service, and would be regression proof because the functionality would not interact with what is already in existence (and thus being used currently).

Let me know what you guys think! I think this approach will be a lot of work, but I think it will unlock huge potential with this module. Consider this an RFC

nateabele · 2013-09-18T12:03:08Z

@toddhgardner Right on, that sounds perfect. One thing @ksperling and I discussed in a separate issue (I'll see if I can find it) is the idea of making type definitions reusable by attaching them to $urlMatcherFactory, something like this:

$urlMatcherFactoryProvider.param("typeName", {
  equals: function (typeObj, otherObj) {},
  decode: function (typeObj) {},
  encode: function (value) {}
});

@thebigredgeek Seems like the most important parts of what you're looking for are accomplishable by combining the above, with two-way binding for $stateParams, which is on the roadmap.

toddhgardner · 2013-09-18T17:00:00Z

@nateabele that sounds great--could totally use that as well, and should be easy enough to add in if we already have support for builtin types like integer and boolean. I'm getting started on this today and will post any questions, etc on this thread.

toddhgardner · 2013-09-18T17:11:07Z

I had to make a few minor environment fixes just to get development started. Do you want me to send a pull request in?

grunt dev is broken, starts the connect:sample task and never starts watching
git is not ignoring bower_components

Also, I see 4 tests failing in Firefox 23.

nateabele · 2013-09-18T17:35:35Z

@toddhgardner Yeah, go ahead, thanks. I normally do my testing with grunt karma and PhantomJS.

nateabele · 2013-11-22T00:36:15Z

@janders223 Well-played, sir. Good on you. I'm fine merging this with the caveat that it's gonna get broken in 0.4 when we finish off typed parameters. That cool with everyone?

nateabele · 2013-11-22T00:37:20Z

@janders223 Alternatively, if you wanna pick up the torch on the typed-parameter PR I can guide you on where to go with it.

FyerPower · 2013-11-22T00:50:51Z

@janders223 and @nateabele I'm fine with that. It atleast solves a need that many seem to be having and if broken in 0.4 we can try to solve the issue again.

timkindberg · 2013-11-22T02:42:44Z

@nateabele just for some context, Jim, Michael and I were all hanging out having some beers. Michael mentioned his complaint about the lack of this feature. I told him to +1 it so we could keep track of popularity. You replied in only a way that you could. Motivated us to see if we could actually add the feature and blam!

thoreinstein · 2013-11-22T03:35:48Z

@nateabele I'm cool with trying to run with that. I would love to give back to a project that I love.

Fixes #125 Add reloadOnSearch property to stateConfig

nateabele · 2013-11-23T15:58:48Z

@gamegenius86 @janders223 Merged #593. To be clear, when typed parameters are implemented, it's not that this feature will go away, it's just that the implementation will be different (read: better :-)).

thoreinstein · 2013-11-24T13:35:38Z

@nateabele Roger that. I should have some time coming to dig into that thread, and look into the typed parameters.

leefernandes · 2013-12-25T00:14:16Z

Did ui.router come up with a solution to allow $state.go() to update url :params w/out reloading the state controller?

timkindberg · 2013-12-28T17:17:08Z

@itsleeowen not yet. Only search params, ?param.

lmessinger · 2014-01-03T13:26:45Z

@timkindberg can you explain? i didnt understand what you mean by Only search params, ?param.

timkindberg · 2014-01-04T02:32:40Z

So if you have:

.state('search', {
  url: '/search?query',
  reloadOnSearch: false
})

Then in you app you can use $location.search({query: 'kittens'}) to change the query param in the url but not reactivate the state over and over.

timkindberg · 2014-01-04T02:34:10Z

The spec may help too, though its not so clear to follow.

ui-router/test/stateSpec.js

Lines 152 to 162 in a100dea

    
           it('doesn\'t trigger state change if reloadOnSearch is false', inject(function ($state, $q, $location, $rootScope){ 
        
             initStateTo(RS); 
        
             $location.search({term: 'hello'}); 
        
             var called; 
        
             $rootScope.$on('$stateChangeStart', function (ev, to, toParams, from, fromParams) { 
        
               called = true 
        
             }); 
        
             $q.flush(); 
        
             expect($location.search()).toEqual({term: 'hello'}); 
        
             expect(called).toBeFalsy();         
        
           }));

lmessinger · 2014-01-04T02:48:28Z

Thanks. Well I did do some tests and it seems reloadOnSearch=false works also for URL params. Here's a plunk - http://plnkr.co/edit/2I8jFk?p=preview

I can then listen on locationChangeSuccess event for changes on parameters.

One issue, of course, is how to extract the parameters from the location.path(). Any ideas, by any chance?

timkindberg · 2014-01-04T03:23:04Z

Interesting! It wasn't really meant to work with path params, so you are kind of on your own with that one, but you could always parse them yourself from the $location.path() value.

msafi · 2014-04-01T16:11:30Z

@timkindberg If it's not meant to work with path params, does that make it a bug?

In my case, I want to prevent reload on search, but still reload on param changes. I can't do that because of this issue.

timkindberg · 2014-04-02T02:34:21Z

Yeah technically, but it will probably be fixed by just rewriting the parameter stuff. We are gonna do typed parameters one of these days.

nateabele · 2014-04-22T14:06:43Z

Between #1032 and the stuff in the dynamic-params branch (specifically, 7a82df7), this is resolved now.

sraparla · 2014-12-19T14:29:21Z

We were using 0.2.10, updating to 0.2.13 fixed it without us having to do anything about it.

NULL-OPERATOR · 2017-04-07T17:49:51Z

yeh

nateabele closed this as completed in cba5233 Nov 23, 2013

nateabele added a commit that referenced this issue Nov 23, 2013

Merge pull request #593 from janders223/issue-125

4175179

Fixes #125 Add reloadOnSearch property to stateConfig

timkindberg mentioned this issue Jan 5, 2014

Using $location.hash() #510

Closed

nateabele mentioned this issue Mar 13, 2014

RFC: Default Query String Params #938

Closed

edrmp mentioned this issue Apr 8, 2014

Empty $location.search() if move from v0.0.1 to v0.2.0 #385

Closed

nateabele reopened this Apr 9, 2014

nateabele closed this as completed Apr 22, 2014

mfzl mentioned this issue Jun 25, 2014

Can we pass objects these days with transitionTo ? #983

Closed

christopherthielen modified the milestones: 0.2.12, 1.5.0 Nov 16, 2014

shavo007 mentioned this issue Sep 29, 2015

Child state with stateParams will load controller twice even if reloadOnSearch is false #2272

Closed

[bug] Child state with stateParams will load controller twice if stateParam absent #125

[bug] Child state with stateParams will load controller twice if stateParam absent #125

Comments

carlatposse commented May 8, 2013

ksperling commented May 11, 2013

ksperling commented May 24, 2013

nateabele commented May 24, 2013

nateabele commented May 24, 2013

paullryan commented May 24, 2013

nateabele commented May 24, 2013

timkindberg commented May 24, 2013

paullryan commented May 24, 2013

nateabele commented May 24, 2013

paullryan commented May 24, 2013

ksperling commented May 24, 2013

nateabele commented May 24, 2013

ksperling commented May 25, 2013

nateabele commented May 25, 2013

toddhgardner commented Sep 17, 2013

thebigredgeek commented Sep 18, 2013

nateabele commented Sep 18, 2013

toddhgardner commented Sep 18, 2013

toddhgardner commented Sep 18, 2013

nateabele commented Sep 18, 2013

nateabele commented Nov 22, 2013

nateabele commented Nov 22, 2013

FyerPower commented Nov 22, 2013

timkindberg commented Nov 22, 2013

thoreinstein commented Nov 22, 2013

nateabele commented Nov 23, 2013

thoreinstein commented Nov 24, 2013

leefernandes commented Dec 25, 2013 • edited Loading

timkindberg commented Dec 28, 2013

lmessinger commented Jan 3, 2014

timkindberg commented Jan 4, 2014

timkindberg commented Jan 4, 2014

lmessinger commented Jan 4, 2014

timkindberg commented Jan 4, 2014

msafi commented Apr 1, 2014

timkindberg commented Apr 2, 2014

nateabele commented Apr 22, 2014

sraparla commented Dec 19, 2014

NULL-OPERATOR commented Apr 7, 2017

leefernandes commented Dec 25, 2013 •

edited

Loading