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

chore($sniffer): Remove $sniffer.hashchange #9497

Merged
merged 1 commit into from
Oct 8, 2014
Merged

chore($sniffer): Remove $sniffer.hashchange #9497

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 1 addition & 3 deletions src/ng/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,7 @@ function Browser(window, document, $log, $sniffer) {
// html5 history api - popstate event
if ($sniffer.history) jqLite(window).on('popstate', fireUrlChange);
// hashchange event
if ($sniffer.hashchange) jqLite(window).on('hashchange', fireUrlChange);
// polling
else self.addPollFn(fireUrlChange);
jqLite(window).on('hashchange', fireUrlChange);

urlChangeInit = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a bit of code that's now not used that could be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is still needed for ngCookies, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

eventually I'd like to get this out of browser and move it to cookies, but now is not the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

url polling isn't needed for ngCookies, afaik

Copy link
Member Author

Choose a reason for hiding this comment

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

@caitp not URL pooling, just $browser.addPollFn.

Obviously it could be moved to ngCookies but see what @IgorMinar said above.

Copy link
Contributor

Choose a reason for hiding this comment

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

this CL doesn't get rid of addPollFn though. (unless I'm not seeing something in the diff?)

Maybe I'm just misunderstanding the conversation, I thought you were saying "don't remove this, ngCookies needs it", but the thing ngCookies needs is not removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. @realityking suggested more code can be removed, referring to $browser.addPollFn. I noted that it's still used in ngCookies co cannot be removed; @IgorMinar said it can be moved to ngCookies but not now.

Copy link
Member Author

Choose a reason for hiding this comment

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

IOW This in This is still needed for ngCookies referred to $browser.addPollFn, not the part removed here; sorry I wasn't more clear!

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I had missed that itt was used in ngCookies.

}
Expand Down
4 changes: 0 additions & 4 deletions src/ng/sniffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
* @requires $document
*
* @property {boolean} history Does the browser support html5 history api ?
* @property {boolean} hashchange Does the browser support hashchange event ?
* @property {boolean} transitions Does the browser support CSS transition events ?
* @property {boolean} animations Does the browser support CSS animation events ?
*
Expand Down Expand Up @@ -65,9 +64,6 @@ function $SnifferProvider() {
// jshint -W018
history: !!($window.history && $window.history.pushState && !(android < 4) && !boxee),
// jshint +W018
hashchange: 'onhashchange' in $window &&
// IE8 compatible mode lies
(!documentMode || documentMode > 7),
hasEvent: function(event) {
// IE9 implements 'input' event it's so fubared that we rather pretend that it doesn't have
// it. In particular the event is not fired when backspace or delete key are pressed or
Expand Down
60 changes: 5 additions & 55 deletions test/ng/browserSpecs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ describe('browser', function() {
beforeEach(function() {
scripts = [];
removedScripts = [];
sniffer = {history: true, hashchange: true};
sniffer = {history: true};
fakeWindow = new MockWindow();
fakeDocument = new MockDocument();

Expand Down Expand Up @@ -597,7 +597,7 @@ describe('browser', function() {
var currentHref;

beforeEach(function() {
sniffer = {history: true, hashchange: true};
sniffer = {history: true};
currentHref = fakeWindow.location.href;
});

Expand Down Expand Up @@ -683,9 +683,8 @@ describe('browser', function() {
expect(callback).toHaveBeenCalledOnce();
});

it('should forward only popstate event when both history and hashchange supported', function() {
it('should forward only popstate event when history supported', function() {
sniffer.history = true;
sniffer.hashchange = true;
browser.onUrlChange(callback);
fakeWindow.location.href = 'http://server/new';

Expand All @@ -697,9 +696,8 @@ describe('browser', function() {
expect(callback).toHaveBeenCalledOnce();
});

it('should forward hashchange event with new url when only hashchange supported', function() {
it('should forward hashchange event with new url when history not supported', function() {
sniffer.history = false;
sniffer.hashchange = true;
browser.onUrlChange(callback);
fakeWindow.location.href = 'http://server/new';

Expand All @@ -711,56 +709,8 @@ describe('browser', function() {
expect(callback).toHaveBeenCalledOnce();
});

it('should use polling when neither history nor hashchange supported', function() {
it('should not fire urlChange if changed by browser.url method', function() {
sniffer.history = false;
sniffer.hashchange = false;
browser.onUrlChange(callback);

fakeWindow.location.href = 'http://server.new';
fakeWindow.setTimeout.flush();
expect(callback).toHaveBeenCalledWith('http://server.new', null);

callback.reset();

fakeWindow.fire('popstate');
fakeWindow.fire('hashchange');
expect(callback).not.toHaveBeenCalled();
});

describe('after an initial location change by browser.url method when neither history nor hashchange supported', function() {
beforeEach(function() {
sniffer.history = false;
sniffer.hashchange = false;
browser.url("http://server/#current");
});

it('should fire callback with the correct URL on location change outside of angular', function() {
browser.onUrlChange(callback);

fakeWindow.location.href = 'http://server/#new';
fakeWindow.setTimeout.flush();
expect(callback).toHaveBeenCalledWith('http://server/#new', null);

fakeWindow.fire('popstate');
fakeWindow.fire('hashchange');
expect(callback).toHaveBeenCalledOnce();
});

});

it('should not fire urlChange if changed by browser.url method (polling)', function() {
sniffer.history = false;
sniffer.hashchange = false;
browser.onUrlChange(callback);
browser.url('http://new.com');

fakeWindow.setTimeout.flush();
expect(callback).not.toHaveBeenCalled();
});

it('should not fire urlChange if changed by browser.url method (hashchange)', function() {
sniffer.history = false;
sniffer.hashchange = true;
browser.onUrlChange(callback);
browser.url('http://new.com');

Expand Down
2 changes: 1 addition & 1 deletion test/ng/locationSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ describe('$location', function() {
};
};
$browserProvider.$get = function($document, $window) {
var sniffer = {history: true, hashchange: false};
var sniffer = {history: true};
var logs = {log:[], warn:[], info:[], error:[]};
var fakeLog = {log: function() { logs.log.push(slice.call(arguments)); },
warn: function() { logs.warn.push(slice.call(arguments)); },
Expand Down
14 changes: 0 additions & 14 deletions test/ng/snifferSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,6 @@ describe('$sniffer', function() {
});
});

describe('hashchange', function() {
it('should be true if onhashchange property defined', function() {
expect(sniffer({onhashchange: true}).hashchange).toBe(true);
});

it('should be false if onhashchange property not defined', function() {
expect(sniffer({}).hashchange).toBe(false);
});

it('should be false if documentMode is 7 (IE8 comp mode)', function() {
expect(sniffer({onhashchange: true}, {documentMode: 7}).hashchange).toBe(false);
});
});


describe('hasEvent', function() {
var mockDocument, mockDivElement, $sniffer;
Expand Down