Skip to content

Core: Don't use .apply() on console.log to satisfy IE9 #241

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

Closed
wants to merge 1 commit into from

Conversation

dmethvin
Copy link
Member

Fixes #231

Not sure of a good way to test this for IE9, which is why the problem existed in the first place.

@mention-bot
Copy link

@dmethvin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mgol, @silentroach and @markelog to be potential reviewers.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 87.854% when pulling d551dce on dmethvin:231-console into 9e3dfcb on jquery:master.

@mgol
Copy link
Member

mgol commented Nov 28, 2016

LGTM. I don't see a way to test it in IE 9 but we could simulate its situation via:

if ( !window.console ) {
	window.console = {};
}
if ( !window.console.log ) {
	window.console.log = function () {};
	window.console.log.__proto__ = null;
}


if ( !log ) {
// IE9 console is a host object, callable but doesn't have .apply()
if ( !window.console || !window.console.log ) {
Copy link
Member

Choose a reason for hiding this comment

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

typeof window.console.apply wouldn't work?

Copy link
Member

Choose a reason for hiding this comment

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

It won't as there is no window.console in IE 9.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, and even when window.console exists in IE9 (when the debugger window is active) it is a "host object" that is callable but doesn't have .apply().

@dmethvin
Copy link
Member Author

@markelog @mgol same for this PR

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.

5 participants