Apply window.console instead of console, which is no longer defined #3686
Conversation
|
Looks good to me |
|
This doesn't seem right to me. The log methods expect to the called on the |
|
You're right. Firefox doesn't like it. I'm gonna go back to what I was doing in my other PR. |
| // Bail out if there's no console. | ||
| if (!fn) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
At this point, we've already stored the arguments in history; so, we can safely bail out if there's nothing to do. This is cheaper than potentially running JSON.stringify and calling a no-op.
There was a problem hiding this comment.
sounds like a better idea
There was a problem hiding this comment.
This also has the benefit of avoiding the previous potentiality of having a no-op assigned to fn when console does not exist then hitting line 70 and calling fn.apply(window.console...), which could cause an error (not entirely sure).
misteroneill
changed the title
| // Bail out if there's no console. | ||
| if (!fn) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
sounds like a better idea
| @@ -62,7 +67,7 @@ export const logByType = (type, args, stringify = !!IE_VERSION && IE_VERSION < 1 | |||
| if (!fn.apply) { | |||
| fn(args); | |||
| } else { | |||
| fn[Array.isArray(args) ? 'apply' : 'call'](console, args); | |||
| fn[Array.isArray(args) ? 'apply' : 'call'](window.console, args); | |||
There was a problem hiding this comment.
I assume using global console caused issues as well? and what about just calling apply and making args an array be default?
There was a problem hiding this comment.
Yeah, the implied global console combined with the noop (which had apply()) was causing the problem in the first place.
As to the other change, I think I want to keep this fix pretty minimal. More changes are more opportunities for bugs!
Description
This fixes the potential for a completely broken player under the following conditions:
consoleis closed, so undefinedvideojs.logor one of its methodsSpecific Changes proposed
No longer attempts to
apply(console, args)becauseconsoleis no longer defined as a local variable after e85c1c0 so the browser looks up the scope until it reaches the global scope and, in IE9, because there is no console unless it's open... throws a "console is undefined" error.Requirements Checklist