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

fix($location): allow empty string URLs to reset path, search, and hash #10064

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@NevilleS
Copy link
Contributor

NevilleS commented Nov 14, 2014

Currently, providing '' to $location#url will only reset the hash, but otherwise has no effect. This
change brings the behaviour of $location#url more inline with window.location.href, which when
assigned to an empty string loads the page's base href.

Before:
$location.url() // http://www.example.com/path
$location.url('') // http://www.example.com/path

After:
$location.url() // http://www.example.com/path
$location.url('') // http://www.example.com

Fixes #10063

fix($location): allow empty string URLs to reset path, search, and hash
Currently, providing '' to $location#url will only reset the hash, but otherwise has no effect. This
change brings the behaviour of $location#url more inline with window.location.href, which when
assigned to an empty string loads the page's base href.

Before:
$location.url() // http://www.example.com/path
$location.url('') // http://www.example.com/path

After:
$location.url() // http://www.example.com/path
$location.url('') // http://www.example.com

Fixes #10063
@NevilleS

This comment has been minimized.

Copy link
Contributor Author

NevilleS commented Nov 14, 2014

This is very basic, and I tested to see that it behaves as I'd expect to, but... after digging in a bit more I realized that $location.url('') doesn't quite do nothing, it resets the hash. So this could potentially break applications that are using this behaviour (although probably not intentionally). I haven't marked it as a breaking change though, let me know what you guys think.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Nov 14, 2014

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@mary-poppins mary-poppins added cla: no and removed cla: yes labels Nov 14, 2014

@NevilleS

This comment has been minimized.

Copy link
Contributor Author

NevilleS commented Nov 14, 2014

@mary-poppins are you still broken? 😢

@pkozlowski-opensource

This comment has been minimized.

Copy link

pkozlowski-opensource commented on src/ng/location.js in 227a327 Nov 15, 2014

Maybe it would be more readable when written like: isDefined(match[1]) ?

This comment has been minimized.

Copy link
Owner Author

NevilleS replied Nov 15, 2014

This comment has been minimized.

Copy link

pkozlowski-opensource replied Nov 15, 2014

Ah, right, should have tried it myself before commenting. Sorry for the noise.

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 15, 2014

@NevilleS the test is good, left one minor comment. Don't feel too strong about it but I find the proposed version more readable. WDYT?

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 15, 2014

OK, cool, my comment is not valid really.

LGTM.

@NevilleS

This comment has been minimized.

Copy link
Contributor Author

NevilleS commented Nov 15, 2014

Thanks. I'm still unsure whether this constitutes a breaking change:
previously using an empty string would only reset the hash, now it resets
path+search+hash. I believe this is the correct behaviour, but since it is
different, should it be flagged as breaking?
On Nov 15, 2014 11:41 AM, "Pawel Kozlowski" notifications@github.com
wrote:

OK, cool, my comment is not valid really.

LGTM.


Reply to this email directly or view it on GitHub
#10064 (comment).

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 15, 2014

Yes, technically it is a breaking change. At the same time if someone is using .url() to reset the hash-part only I would say that this is incorrect usage of the API. In this sense I think this breaking change is fixing a bug that shouldn't be here in the first place. So I don't think I should mark it as a breaking change.

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 22, 2014

@NevilleS did you sign the CLA? There is a new bot that can verify a signature but you need to update your CLA with the GitHub user name: http://angularjs.blogspot.fr/2014/11/angular-cla-infrastructure-changes.html

@NevilleS

This comment has been minimized.

Copy link
Contributor Author

NevilleS commented Nov 22, 2014

Yes, I have. Hopefully @googlebot will agree... (bump bump)

@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 22, 2014

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Nov 22, 2014

@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 22, 2014

Cool, thnx @NevilleS. Merging.

@NevilleS

This comment has been minimized.

Copy link
Contributor Author

NevilleS commented Nov 22, 2014

👍 happy to help

@NevilleS NevilleS deleted the NevilleS:accept-empty-string-url branch Nov 22, 2014

Roman-Didenko added a commit to Roman-Didenko/angular.js that referenced this pull request Mar 31, 2015

fix($location): allow empty string URLs to reset path, search, and hash
Currently, providing '' to $location#url will only reset the hash, but otherwise has no effect. This
change brings the behaviour of $location#url more inline with window.location.href, which when
assigned to an empty string loads the page's base href.

Before:
$location.url() // http://www.example.com/path
$location.url('') // http://www.example.com/path

After:
$location.url() // http://www.example.com/path
$location.url('') // http://www.example.com

Fixes angular#10063

Closes angular#10064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.