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

refactor($browser): wrap Get(history.state) in try/catch #10369

Closed
wants to merge 1 commit into from
Closed

refactor($browser): wrap Get(history.state) in try/catch #10369

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
16 changes: 14 additions & 2 deletions src/ng/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,22 @@ function Browser(window, document, $log, $sniffer) {

// This variable should be used *only* inside the cacheState function.
var lastCachedState = null;
function getCurrentState() {
try {
var state = history.state;
Copy link
Member

Choose a reason for hiding this comment

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

Curious: Why not use window ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we cache history anyways, might as well use it

if (state === void 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should change isDefined() to use this method of testing for undefined and use it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since history.state is either going to be a valid object, undefined or null, or it is going to throw an error we can simply do:

function getCurrentState() {
  try {
    return history.state || null;
  } catch(e) {
    return null;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not going to be either a valid object, undefined or null, eg https://dxr.mozilla.org/mozilla-central/source/dom/webidl/History.webidl#18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could be "false", could be "0", could be the empty string, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically, it can legally be undefined, too... so hmm

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we are pretending that it will never be undefined : https://github.com/angular/angular.js/blob/master/src/ng/browser.js#L159-L164

state = null;
}
return state;
} catch (e) {
// MSIE can reportedly throw when there is no state (UNCONFIRMED).
return null;
}
}

function cacheState() {
// This should be the only place in $browser where `history.state` is read.
cachedState = window.history.state;
cachedState = isUndefined(cachedState) ? null : cachedState;
cachedState = getCurrentState();

// Prevent callbacks fo fire twice if both hashchange & popstate were fired.
if (equals(cachedState, lastCachedState)) {
Expand Down