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

Fixes bug when $location.search() is not returning search part of current url. Inluding tests. #9445

Closed
wants to merge 1 commit into from

Conversation

bullgare
Copy link
Contributor

@bullgare bullgare commented Oct 6, 2014

The issue appears when we set $location.search() with some object and after that change this object's properties, like that:

var obj = {foo: 'bar'};
$location.search(obj); // now url is set to '#?foo=bar'
obj.foo = 'boo';
var currentUrlObj = $location.search();
// current location is still '#?foo=bar' 
// while currentUrlObj is telling that current url object is {foo: 'boo'}
// which isn't correct

There's a little demo onJSBin:
http://jsbin.com/rekana/5/edit?js,output
And another JSBin with behaviour fixed on consumer's side:
http://jsbin.com/rekana/1/edit?js,output
I'm sure it's a bad idea to make someone think of that on the consumer's side.

@bullgare
Copy link
Contributor Author

bullgare commented Oct 6, 2014

@caitp could you please code review this.
It's a copy of this request #9410 where I had some problems with merge.

@bullgare
Copy link
Contributor Author

bullgare commented Oct 8, 2014

@caitp could you please make a code review.

@@ -475,6 +475,7 @@ LocationHashbangInHtml5Url.prototype =
search = search.toString();
this.$$search = parseKeyValue(search);
} else if (isObject(search)) {
search = copy(search, {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should no-op when the passed search object is the same as the one returned from search(), since it would be pointless work

@caitp
Copy link
Contributor

caitp commented Oct 8, 2014

lgtm, but I would add the no-op in the case I mentioned

@@ -475,6 +475,7 @@ LocationHashbangInHtml5Url.prototype =
search = search.toString();
this.$$search = parseKeyValue(search);
} else if (isObject(search)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (search === this.$$search) return this;

@caitp
Copy link
Contributor

caitp commented Oct 8, 2014

@IgorMinar wdyt? I don't think it's a bug that hit's everyone, but it seems kinda buggy to me

@IgorMinar
Copy link
Contributor

Lgtm

@caitp caitp closed this in c7a9009 Oct 9, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants