Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle exceptions on windows phone 8 in cordova. #26

Merged
merged 1 commit into from Jul 3, 2015

Conversation

kitsunde
Copy link
Contributor

@kitsunde kitsunde commented Jul 3, 2015

This is because you cannot access document.domain on windows phone 8.

I also removed the check for .indexOf. This change introduced it:

if (url.indexOf && url.indexOf(document.domain) !== -1) {
it probably wasn't necessary because if url was undefined it was assigned to be an empty string. The current version of this function explicitly checks if url is a string.

String.prototype.indexOf

Is supported in every browser since the dawn of man (JavaScript 1.0), so we don't need need to check the general case.

@niemyjski
Copy link
Collaborator

Thanks for your pull request. I think we need to keep that check for indexOf for two reasons.

  1. We are not specifically verifying that the type is a string (so we don't know if the function exists).
  2. In <es5 browsers the only thing you can assume is that the window object is defined.

Could you please add it back for these reasons and I'll accept this pull request.

@kitsunde
Copy link
Contributor Author

kitsunde commented Jul 3, 2015

@niemyjski

  1. We do know that the type is a string. https://github.com/csnover/TraceKit/blob/master/tracekit.js#L361
  2. You can know that because there's not a single environment that has strings that are missing indexOf that you support. It has been around since IE6, it's part of the original standard.

It's also not a requirement for this codebase to check everything just in the first 150 lines:

Array.prototype.push:
https://github.com/csnover/TraceKit/blob/master/tracekit.js#L112
Array.prototype.splice:
https://github.com/csnover/TraceKit/blob/master/tracekit.js#L122
Function.prototype.call, Array.prototype.concat, Function.prototype.call
https://github.com/csnover/TraceKit/blob/master/tracekit.js#L139

Some of these are later additions than indexOf for strings. It's bizarre to check for these things.

@niemyjski
Copy link
Collaborator

@kitsunde, ahh I just woke up and missed that check, my bad. I just took over this library about a month ago and still learning more about it every day :).

Do you think we should be checking for some of these since they might not be available in ie6?

Thanks for your contribution!

niemyjski added a commit that referenced this pull request Jul 3, 2015
Handle exceptions on windows phone 8 in cordova.
@niemyjski niemyjski merged commit 0e2e471 into csnover:master Jul 3, 2015
@kitsunde
Copy link
Contributor Author

kitsunde commented Jul 3, 2015

@niemyjski I skimmed the source. I see no obvious issues with any of the native methods that's being used currently.

@niemyjski
Copy link
Collaborator

@kitsunde I'll work on putting out a release soon :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants