fix($location): avoid unnecessary `$locationChange*` events due to empty hash #16636

gkalpak · 2018-07-19T16:30:18Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix (among others).

What is the current behavior? (You can also link to an open issue here)
#16632

What is the new behavior (if this is a feature change)?
No unnecessary $locationChange* events due to empty hash

Does this PR introduce a breaking change?
Hopefully not 😁

Please check if the PR fulfills these requirements

The commit message follows our guidelines
~~Fix/Feature: Docs have been added/updated~~
Fix/Feature: Tests have been added; existing tests pass

googlebot · 2018-07-19T16:31:11Z

☹️ Sorry, but only Googlers may change the label cla: yes.

googlebot · 2018-07-19T16:31:11Z

☹️ Sorry, but only Googlers may change the label cla: yes.

jbedard · 2018-07-19T16:43:38Z

src/ng/browser.js

@@ -143,7 +143,7 @@ function Browser(window, document, $log, $sniffer, $$taskTrackerFactory) {
      // - pendingLocation is needed as browsers don't allow to read out
      //   the new location.href if a reload happened or if there is a bug like in iOS 9 (see
      //   https://openradar.appspot.com/22186109).
-      return pendingLocation || location.href;
+      return pendingLocation || trimEmptyHash(location.href);


What about pendingLocation?

pendingLocation always has the empty hash fragment stripped. In a nutshell, pendingLocation is always the value $browser.url(url) is called with, which is always $location.absUrl() (directly or indirectly), which always has the empty hash stripped off.

This potential difference between pendingLocation and location.href is what causes the issue. Because while pendingLocation is defined, $browser.url() return the url without the empty hash, but once pendingLocation gets undefined (i.e. on hashchange, popstate, which may be triggered asynchronously), then $browser.url() starts returning the url with the empty fragment.

So, the returned url changes, even if there is no actual change in the url.

This potential difference between pendingLocation and location.href is what causes the issue

Aha, that helps me understand this a bit more. Why does location.href have the hash in the first place?

That's what browsers do (sometimes 😁). I found that this can happen when clicking a link with a trailing # (but not when directly setting location.hash).

I think I'd vote to still do trimEmptyHash(pendingLocation || location.href) just for consistency...

jbedard · 2018-07-19T16:44:59Z

Where is $browser.url() being used to cause this bug? I question if we should fix it there instead similar to aee7d53

gkalpak · 2018-07-19T17:35:54Z

Where is $browser.url() being used to cause this bug? I question if we should fix it there instead similar to aee7d53

This is one place. There may be others.
Note that is many places we trim the empty hash before using the returned url. (If we decide this fix is desired, I will remove those unnecessary calls.)

Also, this looks a little problematic:

angular.js/src/ng/location.js

Lines 970 to 979 in 6706353

    
           $rootScope.$evalAsync(function() { 
        
             var oldUrl = $location.absUrl(); 
        
             var oldState = $location.$$state; 
        
             var defaultPrevented; 
        
             newUrl = trimEmptyHash(newUrl); 
        
             $location.$$parse(newUrl); 
        
             $location.$$state = newState; 
        
             defaultPrevented = $rootScope.$broadcast('$locationChangeStart', newUrl, oldUrl, 
        
                 newState, oldState).defaultPrevented;

Although we don't trim the empty hash for determining there's been a url change, we trim it before broadcasting the event. So, you will get a $locationChangeStart event from /foo/ to /foo/ 😱

In general, I prefer to address the root of the problem. Patching stuff "just-in-time" to avoid a specific bug makes the code too complicated and hard to follow.

(But I understand $location/$browser might a special case.)

jbedard · 2018-07-19T18:09:01Z

While patching stuff "just-in-time" can be iffy, modifying something generic like $browser.url() for a specific case that might not apply everywhere is also iffy. I think that was one reason we chose not to modify $browser.url() in aee7d53?

For the $locationChangeStart newUrl thing... can we just drop the newUrl = trimEmptyHash(newUrl); line?

gkalpak · 2018-07-19T19:16:25Z

modifying something generic like $browser.url() for a specific case that might not apply everywhere is also iffy.

My point is that the proposed change does apply everywhere and does not change the behavior of $browser other than not incorrectly reporting URL changes.

But I will understand if you feel uncomfortable with changing .url() 😃

jbedard · 2018-07-19T19:22:38Z

But is adding/removing # a URL change? The browser thinks it is... 🤔

gkalpak · 2018-07-19T19:56:07Z

I think you already found #16636 (comment), which (kind of) explains that it's not the browser that thinks there's a change, it's $browser.url() that temporarily thinks so (while pendingLocation is set).

jbedard · 2018-07-19T20:04:28Z

I mean if you append # to the end of a URL and press enter, the browser will push a new entry into the history. So normally browsers do consider / and /# to be different URLs.

I guess I just want to acknowledge that the browser does consider them different, it's only us/$location that treats them the same. IDK if that actually means anything though >_>

I'm curious what other code could be simplified with the current fix... and how the $locationChangeStart event can be fixed. Want to go ahead with this as-is for now? I'm still tempted to try the other method and compare though...

jbedard · 2018-07-25T19:10:29Z

src/ng/location.js

@@ -93,7 +94,7 @@ function stripHash(url) {
 }

 function trimEmptyHash(url) {


Can we remove some calls to this within this file now that $browser does it?

Absolutely!

Note that is many places we trim the empty hash before using the returned url. (If we decide this fix is desired, I will remove those unnecessary calls.)

…used deps, etc)

petebacondarwin · 2018-07-25T21:18:36Z

src/ng/browser.js

@@ -1,5 +1,14 @@
 'use strict';
-/* global stripHash: true */
+/* global getHash: true, stripHash: false, trimEmptyHash: true */


where is getHash used globally?

It is "used" in tests, in a mock window. The feature that relies on it is not actually used atm, so there were no breakages.

See 34fe24f (yes, there is a typo in that commit message 😁).

petebacondarwin · 2018-07-25T21:24:56Z

src/ng/location.js

@@ -1,4 +1,5 @@
 'use strict';
+/* global stripHash: true */


One could move stripHash global into browser.js as well for consistency. Otherwise you have two way importing going on.

stripHash is used more in $location than $browser and other similar helpers (e.g. stripBaseUrl, stripFile) are also in $location. I only moved trimEmptyHash, because it is only used in $browser now.

Even if we had imports/exports (which we don't because everything lives inside the great AngularJS IIFE 😁), I don't see two way importing going on. $location would export stripHash and $browser would import stripHash and export getHash and trimEmptyHash.

(We definitely have enough URL-specific helpers that we could move them into a separate file and "import" from there 😁)

I think it's a little weird having $browser depend on $location. Or was that always the case?

I don't know about "always", but it definitely was the case before this PR 😛
(And still is. This PR does not change anything in that regard.)

petebacondarwin

Couple of minor comments.

The helper is used in `fakeWindow.location.hash`. ATM, no test is using the `hash` getter, so there were no errors.

…pty hash Fixes angular#16632

…e to empty hash

Narretz · 2018-07-27T08:10:13Z

test/ng/locationSpec.js

-        expect($browser.url()).toBe('http://new.com/#');
+      mockUpBrowser({initialUrl: 'http://new.com/#', baseHref: '/'});
+      inject(function($browser, $location, $window) {
+        expect($browser.url()).toBe('http://new.com/');


The (non-public) docs for browser.url() say "Without any argument, this method just returns current value of location.href." Isn't this now wrong as the test shows a difference between url() and location.href?

Good point.

…e to empty hash

…pty hash Fixes #16632 Closes #16636

googlebot added the cla: yes label Jul 19, 2018

gkalpak added type: bug needs: review component: $location and removed cla: yes labels Jul 19, 2018

googlebot added the cla: yes label Jul 19, 2018

gkalpak added this to the 1.7.x milestone Jul 19, 2018

gkalpak mentioned this pull request Jul 19, 2018

ngRouter: Multiple controller initialization when search changed #16632

Closed

3 tasks

jbedard reviewed Jul 19, 2018

View reviewed changes

jbedard reviewed Jul 25, 2018

View reviewed changes

gkalpak added 2 commits July 25, 2018 22:25

refactor($location): minor changes (unused deps, exported globals, un…

7f524c4

…used deps, etc)

refactor($location): remove unnecessary capturing group in RegExp

8f28de4

gkalpak force-pushed the fix-location-on-empty-hash branch 2 times, most recently from 000489f to efb1706 Compare July 25, 2018 19:49

petebacondarwin reviewed Jul 25, 2018

View reviewed changes

petebacondarwin approved these changes Jul 25, 2018

View reviewed changes

gkalpak added 4 commits July 26, 2018 10:57

refactor($browser): correctly export helper used in specs

8053bc4

The helper is used in `fakeWindow.location.hash`. ATM, no test is using the `hash` getter, so there were no errors.

test($location): add assertion

bbf6900

fix($location): avoid unnecessary $locationChange* events due to em…

b085893

…pty hash Fixes angular#16632

fixup! fix($location): avoid unnecessary $locationChange* events du…

c37c2e4

…e to empty hash

fixup! fix($location): avoid unnecessary $locationChange* events du…

cdd1f42

…e to empty hash

gkalpak force-pushed the fix-location-on-empty-hash branch from af2da25 to cdd1f42 Compare July 26, 2018 07:58

fixup! fix($location): avoid unnecessary $locationChange* events du…

d736771

…e to empty hash

Narretz reviewed Jul 27, 2018

View reviewed changes

fixup! fix($location): avoid unnecessary $locationChange* events du…

c3ca532

…e to empty hash

jbedard approved these changes Jul 30, 2018

View reviewed changes

gkalpak closed this in 864c7f0 Jul 30, 2018

gkalpak added a commit that referenced this pull request Jul 30, 2018

fix($location): avoid unnecessary $locationChange* events due to em…

1144b1e

…pty hash Fixes #16632 Closes #16636

gkalpak deleted the fix-location-on-empty-hash branch July 30, 2018 20:41

dependencies bot mentioned this pull request Aug 9, 2018

Update 6 dependencies from npm JetBrains/ring-ui#349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($location): avoid unnecessary `$locationChange*` events due to empty hash #16636

fix($location): avoid unnecessary `$locationChange*` events due to empty hash #16636

gkalpak commented Jul 19, 2018

googlebot commented Jul 19, 2018

googlebot commented Jul 19, 2018

jbedard Jul 19, 2018

gkalpak Jul 19, 2018

jbedard Jul 19, 2018

gkalpak Jul 19, 2018

jbedard Jul 25, 2018

gkalpak Jul 25, 2018

jbedard commented Jul 19, 2018

gkalpak commented Jul 19, 2018

jbedard commented Jul 19, 2018

gkalpak commented Jul 19, 2018

jbedard commented Jul 19, 2018

gkalpak commented Jul 19, 2018

jbedard commented Jul 19, 2018

jbedard Jul 25, 2018

gkalpak Jul 25, 2018

petebacondarwin Jul 25, 2018

gkalpak Jul 26, 2018

petebacondarwin Jul 25, 2018

gkalpak Jul 26, 2018

jbedard Jul 30, 2018

gkalpak Jul 30, 2018 •

edited

Loading

petebacondarwin left a comment

Narretz Jul 27, 2018

gkalpak Jul 27, 2018

gkalpak Jul 27, 2018

		@@ -93,7 +94,7 @@ function stripHash(url) {
		}

		function trimEmptyHash(url) {

fix($location): avoid unnecessary $locationChange* events due to empty hash #16636

fix($location): avoid unnecessary $locationChange* events due to empty hash #16636

Conversation

gkalpak commented Jul 19, 2018

googlebot commented Jul 19, 2018

googlebot commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jul 19, 2018

gkalpak commented Jul 19, 2018

jbedard commented Jul 19, 2018

gkalpak commented Jul 19, 2018

jbedard commented Jul 19, 2018

gkalpak commented Jul 19, 2018

jbedard commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix($location): avoid unnecessary `$locationChange*` events due to empty hash #16636

fix($location): avoid unnecessary `$locationChange*` events due to empty hash #16636

gkalpak Jul 30, 2018 •

edited

Loading