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

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

Closed
gkalpak wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7f524c4
refactor($location): minor changes (unused deps, exported globals, un…
gkalpak Jul 19, 2018
8f28de4
refactor($location): remove unnecessary capturing group in RegExp
gkalpak Jul 19, 2018
8053bc4
refactor($browser): correctly export helper used in specs
gkalpak Jul 19, 2018
bbf6900
test($location): add assertion
gkalpak Jul 19, 2018
b085893
fix($location): avoid unnecessary `$locationChange*` events due to em…
gkalpak Jul 19, 2018
c37c2e4
fixup! fix($location): avoid unnecessary `$locationChange*` events du…
gkalpak Jul 25, 2018
cdd1f42
fixup! fix($location): avoid unnecessary `$locationChange*` events du…
gkalpak Jul 25, 2018
d736771
fixup! fix($location): avoid unnecessary `$locationChange*` events du…
gkalpak Jul 26, 2018
c3ca532
fixup! fix($location): avoid unnecessary `$locationChange*` events du…
gkalpak Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! fix($location): avoid unnecessary $locationChange* events du… 
…e to empty hash
  • Loading branch information
@gkalpak
gkalpak committed Jul 26, 2018
commit c37c2e4656c75b375cbe7b3bb0f456bf045a1a41
8 changes: 6 additions & 2 deletions src/ng/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
'use strict';
/* global getHash: true, stripHash: false, trimEmptyHash: false */
/* global getHash: true, stripHash: false, trimEmptyHash: true */
Copy link
Contributor

@petebacondarwin petebacondarwin Jul 25, 2018

Choose a reason for hiding this comment

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

where is getHash used globally?

Copy link
Member Author

@gkalpak gkalpak Jul 26, 2018

Choose a reason for hiding this comment

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

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 😁).


function getHash(url) {
var index = url.indexOf('#');
return index === -1 ? '' : url.substr(index);
}

function trimEmptyHash(url) {
return url.replace(/#$/, '');
}

/**
* ! This is a private undocumented service !
*
Expand Down Expand Up @@ -143,7 +147,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 || trimEmptyHash(location.href);
return trimEmptyHash(pendingLocation || location.href);
}
};

Expand Down
15 changes: 4 additions & 11 deletions src/ng/location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
'use strict';
/* global stripHash: true, trimEmptyHash: true */
/* global stripHash: true */
Copy link
Contributor

@petebacondarwin petebacondarwin Jul 25, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@gkalpak gkalpak Jul 26, 2018

Choose a reason for hiding this comment

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

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 😁)

Copy link
Contributor

@jbedard jbedard Jul 30, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@gkalpak gkalpak Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.)


var PATH_MATCH = /^([^?#]*)(\?([^#]*))?(#(.*))?$/,
DEFAULT_PORTS = {'http': 80, 'https': 443, 'ftp': 21};
Expand Down Expand Up @@ -95,17 +95,11 @@ function stripBaseUrl(base, url) {
}
}


function stripHash(url) {
var index = url.indexOf('#');
return index === -1 ? url : url.substr(0, index);
}

function trimEmptyHash(url) {
return url.replace(/#$/, '');
}


function stripFile(url) {
return url.substr(0, stripHash(url).lastIndexOf('/') + 1);
}
Expand Down Expand Up @@ -944,7 +938,7 @@ function $LocationProvider() {


// rewrite hashbang url <> html5 url
if (trimEmptyHash($location.absUrl()) !== trimEmptyHash(initialUrl)) {
if ($location.absUrl() !== initialUrl) {
$browser.url($location.absUrl(), true);
}

Expand All @@ -963,7 +957,6 @@ function $LocationProvider() {
var oldUrl = $location.absUrl();
var oldState = $location.$$state;
var defaultPrevented;
newUrl = trimEmptyHash(newUrl);
$location.$$parse(newUrl);
$location.$$state = newState;

Expand Down Expand Up @@ -991,8 +984,8 @@ function $LocationProvider() {
if (initializing || $location.$$urlUpdatedByLocation) {
$location.$$urlUpdatedByLocation = false;

var oldUrl = trimEmptyHash($browser.url());
var newUrl = trimEmptyHash($location.absUrl());
var oldUrl = $browser.url();
var newUrl = $location.absUrl();
var oldState = $browser.state();
var currentReplace = $location.$$replace;
var urlOrStateChanged = !urlsEqual(oldUrl, newUrl) ||
Expand Down
4 changes: 2 additions & 2 deletions src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

/* global routeToRegExp: false */
/* global routeToRegExp: false, trimEmptyHash: false */

/**
* @ngdoc object
Expand Down Expand Up @@ -225,7 +225,7 @@ angular.mock.$Browser.prototype = {
state = null;
}
if (url) {
this.$$url = url;
this.$$url = trimEmptyHash(url);
// Native pushState serializes & copies the object; simulate it.
this.$$state = angular.copy(state);
return this;
Expand Down