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

remove log() #1107

Closed
wants to merge 3 commits into from
Closed

remove log() #1107

wants to merge 3 commits into from

Conversation

mathiasbynens
Copy link
Member

Got a great comment on my log() post that pointed out the obvious fact that using the wrapper changes the reported line position of your logs.

This makes clicking through to the Scripts panel far less useful, as well as losing context that a glance at the logged lines can give you.

I think it's best to remove log() and recommend full use of console.log() with the stubbed versions for IE7.

(while we're at it, less add timeStamp to the console stub script.)

@paulirish
Copy link
Member Author

The stubbed versions being the if (! window.console) console.log = function(){} -like thing that we have here: plugins.js#L5-7

@mathiasbynens
Copy link
Member

If we’re assuming jQuery anyway, how about something like this?

// Avoid `console` errors in browsers that lack a console
if (!(window.console && console.log)) {
    window.console = {};
    $.each(['error', 'warn', 'info', 'debug', 'log'], function() {
        window.console[this] = $.noop;
    });
}

It’s easy enough to do without using $.each if the dependency is an issue.

@necolas
Copy link
Member

necolas commented May 29, 2012

We avoided the Modernizr dependency for the GA snippet, so we should probably avoid a jQuery dependency here too.

@paulirish
Copy link
Member Author

Yeah, our console noop is already pretty damn terse.

@devinrhode2
Copy link
Contributor

I was just about to raise this same issue.. I tried to see if there's some sort of closure that can amend this.. but I couldn't figure out anything.

@devinrhode2
Copy link
Contributor

I've found this little abstraction to be really helpful. I mean, I very rarely ever click the line number on my log statements. I'll click it on syntax errors sometimes, but not often.

@necolas
Copy link
Member

necolas commented Jul 7, 2012

Anyone want to have a go at doing this?

@mathiasbynens
Copy link
Member

Here’s the obvious sans-jQuery adaptation of my earlier suggestion:

// Avoid `console` errors in browsers that lack a console
if (!(window.console && console.log)) {
    (function() {
        var noop = function() {},
            methods = ['error', 'warn', 'info', 'debug', 'log'],
            index = -1,
            length = methods.length;
        console = window.console = {};
        while (++index < length) {
            console[methods[index]] = noop;
        }
    }());
}

If this looks good I’ll commit it following the style guide (I’m using tabs here).

Here’s an alternate version that looks prettier IMHO:

// Avoid `console` errors in browsers that lack a console
(window.console && console.log) || (function() {
  var noop = function() {},
      methods = ['error', 'warn', 'info', 'debug', 'log'],
      index = -1,
      length = methods.length,
      console = window.console = {};
  while (++index < length) {
    console[methods[index]] = noop;
  }
}());

@necolas
Copy link
Member

necolas commented Jul 7, 2012

Thanks. Pull request!

@sindresorhus
Copy link
Member

A bit smaller:

if (!(window.console && console.log)) {
    (function() {
        var noop = function() {},
            methods = ['error', 'warn', 'info', 'debug', 'log'],
            i = methods.length,
            console = window.console = {};
        while ( i--  ) {
            console[methods[i]] = noop;
        }
    }());
}

@necolas
Copy link
Member

necolas commented Jul 7, 2012

P.S. I vote for multiple var statements.

@sindresorhus
Copy link
Member

m2

@mathiasbynens
Copy link
Member

Pull request attached. I’ve added all the console API methods that were there in the old version, plus added some that were missing (including timeStamp, as @paulirish suggested). Personally I’d prefer to keep it simple and only stub the most commonly used methods (log, error, debug, and warn?), but I guess it doesn’t really matter.

Note: multiple commits, so please squash them together before merging.

var length = methods.length;
var console = window.console = {};
while (length--) {
console[methods[index]] = noop;

This comment was marked as abuse.

This comment was marked as abuse.

@necolas
Copy link
Member

necolas commented Jul 8, 2012

Thanks! Please could you squash these commits.

mathiasbynens added a commit that referenced this pull request Jul 8, 2012
Remove the previous `log()` wrapper method as it changed the reported
line position in logs. Instead, avoid `console` errors in browsers
without `console` by setting its methods to empty functions.

Ref. #1107.
@necolas
Copy link
Member

necolas commented Jul 8, 2012

Merged into master in 5370479

@necolas necolas closed this Jul 8, 2012
@bgrins
Copy link
Member

bgrins commented Apr 15, 2013

Any thoughts on re-adding log if the line number problem isn't still an issue? The log-full.js works in this way with the line number: https://gist.github.com/bgrins/5108712. I always end up adding this in my projects first thing anyway, so it would be great if it was part of the boilerplate.

@devinrhode2
Copy link
Contributor

I actually just figured out how you can get the click through back, basically if you display the string "url:lineNo" you can click it and have the same effect as the devtools link. So I'll be doing something where I temporarily abuse onerror to get the file url and line number, and then re-console log with the original args and the file url:lineNumber The project I'm working on is close to a 1.0 beta or 0.6, and the sub-component that will do this is here devinrhode2/historicalConsole.js#3

@vdakalov
Copy link

even shorter))

(0&&"")||(function(m,c){while(m.length){c[m.shift()]=function(){};}}(['error','warn','info','debug','log'],window.c={}));

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

Successfully merging this pull request may close these issues.

7 participants