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

Fix broken logDebug() in IE9 and below #473

Merged
merged 6 commits into from
Jan 15, 2016

Conversation

gabalafou
Copy link

No description provided.

@gabalafou gabalafou changed the title Fix broken logDebug() in IE less than v10 Fix broken logDebug() in IE9 and below Jan 15, 2016
@benvinegar
Copy link
Contributor

A wild @gabalafou appears.

@benvinegar
Copy link
Contributor

Have you tested this w/ the console plugin (plugins/console.js)? The purpose here was to use the original prototype method – not just the console object – in case log or warn itself is overwritten by Raven and we end up in some kind of infinite loop.

@gabalafou
Copy link
Author

Funny-- that file notes the same behavior about IE9:

// IE9 doesn't allow calling apply on console functions directly

Alternatively, you just don't try to log anything when the original throws? i.e.,--

try { /* log thingy */ } catch (err) { /* skip debug logging IE<10 */ }

@gabalafou
Copy link
Author

I just copied what console.js does

@benvinegar
Copy link
Contributor

For background, this was the original problem: #373

And my patch: #375

I think ... "skip debug logging in IE<10" is a potential solution. Does this strictly affect apply? What about call?

@gabalafou
Copy link
Author

It affects both apply and call. (I checked.)

Not sure if you submitted your comment before my most recent update (looks like they may have crossed each other).

@benvinegar
Copy link
Contributor

We can't use Function.prototype.bind – it's not supported by IE8 (which Raven.js is supposed to support).

I'd probably try and find a way to remove the bind call from console.js too. I imagine this was added mistakenly, but at least you have to opt-in to using plugins.

Gabriel Fouasnon added 2 commits January 15, 2016 15:32
This reverts commit 9c63e36.
@benvinegar
Copy link
Contributor

@gabalafou – if you change it to bail out if it cannot call the original prototype method, e.g.

try { /* log thingy */ } catch (err) { /* skip debug logging IE<10 */ }

I'll accept and merge. Thanks for your help btw.

@gabalafou
Copy link
Author

IE8 may not have Function.prototype.bind, but it does have Function.prototype.apply-- check out my latest update :)

@benvinegar
Copy link
Contributor

Brilliant. Let me just verify this and we're golden.

benvinegar added a commit that referenced this pull request Jan 15, 2016
Fix broken logDebug() in IE9 and below
@benvinegar benvinegar merged commit 35a298b into getsentry:master Jan 15, 2016
@gabalafou gabalafou deleted the fix-debug-ie9 branch January 15, 2016 23:54
@benvinegar
Copy link
Contributor

Hmm, in hindsight, I really should've asked you to squash these first. Oh well.

@mattrobenolt
Copy link
Contributor

Should we also change this in the console plugin?

@benvinegar
Copy link
Contributor

@mattrobenolt – Yeah.

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.

3 participants