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

fix($location): return '/' for root path in hashbang mode #5712

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Jan 9, 2014

Before this change, on the root of the application, $location.path() would return the empty string. Following this change, it will always return a root of '/'.

Closes #5650

Before this change, on the root of the application, $location.path() would return
the empty string. Following this change, it will always return a root of '/'.

Closes angular#5650
@ghost ghost assigned IgorMinar Jan 10, 2014
@IgorMinar IgorMinar closed this in 63cd873 Jan 10, 2014
IgorMinar added a commit that referenced this pull request Jan 10, 2014
This reverts commit 63cd873.

The change breaks existing tests of Google apps. The problem is that
while we tried to avoid adding #/ to window.location.href unnecessarily
we failed doing so. Likely because by setting $path, at some point
(during a digest) we try to check if $location changed and we mistake the
default '/' with an explicit settign of the path via the `path()` method.
This results in us writing the url with '#/' into $browser.url() which updates
the window.location by adding "#/" to the url - something we tried to avoid
in the first place.

I'll reopen PR #5712.
@IgorMinar IgorMinar reopened this Jan 10, 2014
@IgorMinar
Copy link
Contributor

I hacked up an incomplete fix for the issue due to which the commit got reverted:

diff --git a/src/ng/location.js b/src/ng/location.js
index 0a47445..b8ce427 100644
--- a/src/ng/location.js
+++ b/src/ng/location.js
@@ -609,6 +609,7 @@ function $LocationProvider(){
       LocationMode = $sniffer.history ? LocationHtml5Url : LocationHashbangInHtml5Url;
     } else {
       appBase = stripHash(initialUrl);
+      initialUrl = (initialUrl.indexOf('#') === -1) ? initialUrl + '#/' : initialUrl;
       LocationMode = LocationHashbangUrl;
     }
     $location = new LocationMode(appBase, '#' + hashPrefix);
@@ -679,6 +680,7 @@ function $LocationProvider(){
     var changeCounter = 0;
     $rootScope.$watch(function $locationWatch() {
       var oldUrl = $browser.url();
+      if (oldUrl.indexOf('#') === -1) oldUrl = oldUrl + '#/';
       var currentReplace = $location.$$replace;

       if (!changeCounter || oldUrl != $location.absUrl()) {

it seems that we'll need to change all tests in Angular that expect $location.path() to return an empty string, once we fix this properly this should never be possible. $location.path() should always something that matches /\/.*/.

I consider this to be a breaking change, so we should reschedule this fix for 1.3

@caitp
Copy link
Contributor Author

caitp commented Jan 10, 2014

Ah, understood

@gsklee
Copy link
Contributor

gsklee commented Jan 11, 2014

Just a headsup, I'm seeing this being listed inside the 1.2.8 change log.

@caitp
Copy link
Contributor Author

caitp commented Jan 11, 2014

Yes, unfortunately the reversion doesn't remove it from the changelog, and doesn't get listed itself. hmm

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
Before this change, on the root of the application, $location.path() would return
the empty string. Following this change, it will always return a root of '/'.

Closes angular#5650
Closes angular#5712
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
This reverts commit 63cd873.

The change breaks existing tests of Google apps. The problem is that
while we tried to avoid adding #/ to window.location.href unnecessarily
we failed doing so. Likely because by setting $path, at some point
(during a digest) we try to check if $location changed and we mistake the
default '/' with an explicit settign of the path via the `path()` method.
This results in us writing the url with '#/' into $browser.url() which updates
the window.location by adding "#/" to the url - something we tried to avoid
in the first place.

I'll reopen PR angular#5712.
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
Before this change, on the root of the application, $location.path() would return
the empty string. Following this change, it will always return a root of '/'.

Closes angular#5650
Closes angular#5712
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
This reverts commit 63cd873.

The change breaks existing tests of Google apps. The problem is that
while we tried to avoid adding #/ to window.location.href unnecessarily
we failed doing so. Likely because by setting $path, at some point
(during a digest) we try to check if $location changed and we mistake the
default '/' with an explicit settign of the path via the `path()` method.
This results in us writing the url with '#/' into $browser.url() which updates
the window.location by adding "#/" to the url - something we tried to avoid
in the first place.

I'll reopen PR angular#5712.
@caitp
Copy link
Contributor Author

caitp commented Feb 21, 2014

Just as a note, #5977 has a tiny bit of code in it which will need to be altered here due to the breaking change.

@btford btford removed this from the 1.3.0-rc.5 milestone Oct 8, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0-rc.6, 1.3.0 Oct 13, 2014
@tbosch tbosch modified the milestones: 1.3.x, 1.3.1, Backlog Oct 15, 2014
@caitp caitp closed this Aug 6, 2015
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.

Hashbang routing - Enter site at root path ("/")
5 participants