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
Next Next commit
refactor($location): minor changes (unused deps, exported globals, un… 
…used deps, etc)
  • Loading branch information
@gkalpak
gkalpak committed Jul 25, 2018
commit 7f524c4db39ee3b0b1a253461dae945585954f2b
1 change: 1 addition & 0 deletions src/ng/location.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict';
/* 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
10 changes: 5 additions & 5 deletions test/ng/locationSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,10 +693,10 @@ describe('$location', function() {

describe('location watch', function() {

it('should not update browser if only the empty hash fragment is cleared by updating the search', function() {
it('should not update browser if only the empty hash fragment is cleared', function() {
initService({supportHistory: true});
mockUpBrowser({initialUrl:'http://new.com/a/b#', baseHref:'/base/'});
inject(function($rootScope, $browser, $location) {
mockUpBrowser({initialUrl: 'http://new.com/a/b#', baseHref: '/base/'});
inject(function($browser, $rootScope) {
$browser.url('http://new.com/a/b');
var $browserUrl = spyOnlyCallsWithArgs($browser, 'url').and.callThrough();
$rootScope.$digest();
Expand All @@ -707,8 +707,8 @@ describe('$location', function() {

it('should not replace browser url if only the empty hash fragment is cleared', function() {
initService({html5Mode: true, supportHistory: true});
mockUpBrowser({initialUrl:'http://new.com/#', baseHref: '/'});
inject(function($browser, $location) {
mockUpBrowser({initialUrl: 'http://new.com/#', baseHref: '/'});
inject(function($browser, $location, $window) {
expect($browser.url()).toBe('http://new.com/#');
expect($location.absUrl()).toBe('http://new.com/');
});
Expand Down