Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong URL generated by UrlMatcherFactory format #1543

Closed
wawyed opened this issue Nov 15, 2014 · 7 comments
Closed

Wrong URL generated by UrlMatcherFactory format #1543

wawyed opened this issue Nov 15, 2014 · 7 comments
Assignees
Labels
Milestone

Comments

@wawyed
Copy link
Contributor

wawyed commented Nov 15, 2014

Having 2 states defined as:

var state1 = { name: 'home', url: '/home?param1', controller: function() { }, templateUrl: 'home.html'}
var state2 = { name: 'home.foo', url: '/foo/:id', controller: function() { }, templateUrl: 'foo.html'}

When navigating to state2 (home.foo) with the parameters ({id: "idValue", param1: "param1Value"}) I would expect the URL to look something like:

home/foo/idValue?param1=param1Value.

This is correct and true for 0.2.11, but since 0.2.12 there seems to be a bug with the format of the URL since the generated URL looks like this:

home/foo/param1Value?id=idValue

Which doesn't make any sense since id is not a query string parameter.

Looking at the code the change that seems to be introducing this error is in the function parameters() being called in UrlMatcher.prototype.format at line 989 in 0.2.12 or at line 906 in 0.2.11.

0.2.11 calls the method objectKeys(this.params) which returns the parameters in a different order than 0.2.12 method this.params.$$keys() and that seems to be causing an error in the order the parameters get appended to the generated URL.

Plunkers reproducing the issue:

0.2.11: http://plnkr.co/edit/0L4cLM?p=preview
0.2.12: http://plnkr.co/edit/iVGuKU?p=preview

@christopherthielen
Copy link
Contributor

Thanks, I'll take a look as soon as I have time.

@christopherthielen
Copy link
Contributor

Thanks for the bug report, this is a major bug that I missed. I think the fix for this bug warrants a 0.2.13 release.

@christopherthielen christopherthielen added this to the 0.2.13 milestone Nov 17, 2014
@christopherthielen
Copy link
Contributor

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

fixed plunk

@wawyed
Copy link
Contributor Author

wawyed commented Nov 17, 2014

When can we expect 0.2.13? 👍

@christopherthielen
Copy link
Contributor

Should be just a couple days away

@NevilleS
Copy link

Just ran into this as well. Thanks for the quick fix!

@kumarharsh
Copy link

Man... this was devilishly difficult to find... I was so pissed with my code - why was it breaking all of a sudden when it worked just days ago.... Finally stumbled upon this issue...

Thanks for solving it! Keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants