ehynds / grunt-remove-logging

Grunt.js task for removing console logging statements
MIT License
192 stars 33 forks source link

removal is overly greedy when handling closing braces #6

Open timotheeg opened 11 years ago

timotheeg commented 11 years ago

Adding the following test breaks and result in a syntax error in the resulting processed javascript:

[
  'function foo(msg){console.log(msg)}\nfunction bar(){}',
  {},
  'function foo(msg){}\nfunction bar(){}',
],

incorrectly generates:

function foo(msg){{}
timotheeg commented 11 years ago

real world file that gets broken because of said issue: https://github.com/cloudhead/less.js/blob/master/lib/less/browser.js

I'm not saying the js in that file is good btw (adding a semi-colon after the console.log statement fixes the issue), but projects may import less.js and have their grunt build break because of the issue mentioned.

SerafinDinges commented 10 years ago

This issue especially arises when using the removelogging with uglified files. Here the trailing semicolon gets removed when there is no need for them.

For example:

fail(function(a,b,c){console.log("Error"),console.log(a),console.log(b),console.log(c)})});foo(bar);

The expected result would be:

fail(function(a,b,c){(0);})});foo(bar);

but is instead:

fail(function(a,b,c){foo(bar);

This annoyingly prevents us from including your removelogging in our grunt workflow. A fix would be much appreciated. And thanks anyway for your work.

ehynds commented 10 years ago

Yeah, one limitation of this plugin is that it requires a trailing semicolon since the regex is non-greedy. Until I can figure out how to fix that (pulls welcome) I suggest running this task before minification.

timotheeg commented 10 years ago

The heart of the issue, imho, is that the removing log statements cannot be done reliably with regexps alone. But I do realize that adding a full js tokenizer in there is a big job :(

Soviut commented 10 years ago

I too have been having issues doing a removelogging after uglify. The current solution I'm using is to just put removelogging before the uglify task in the build order. This feels fragile though.