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

unused functions are not removed #743

Closed
edwardcoolson opened this issue Jun 23, 2015 · 13 comments

Comments

Projects
None yet
5 participants
@edwardcoolson
Copy link

commented Jun 23, 2015

I'm new to uglifyJS, it seems that it does not remove unused functions. I have this simple code:

function isEmpty(o)
{
  for(var key in o)
    if(o.hasOwnProperty(key))
      return false;
  return true;
} 

function isBrowserIE()
{
  return navigator.userAgent.toLowerCase().indexOf('msie') > -1;
}

var o = {};
o.prop1 = 5;
o.prop2 = 10;

var b = isEmpty(o);
alert(b);

I've minified this code online here: http://lisperator.net/uglifyjs/
and got the following code:

function isEmpty(r){for(var o in r)if(r.hasOwnProperty(o))return!1
return!0}function isBrowserIE(){return navigator.userAgent.toLowerCase().indexOf("msie")>-1}var o={}
o.prop1=5,o.prop2=10
var b=isEmpty(o)
alert(b)

As anyone can see, the unused function isBrowserIE() is still there. I used default compression options. The compression option Discard unused variables/functions was turned on by default.

Is it a bug or just kept for future releases?
Thank you.

@Skalman

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2015

Your code defines a function isBrowserIE() on the global object, and if Uglify removed the function it would no longer be globally defined – i.e. the code would do something different. Try wrapping everything in an IIFE so that the function isn't globally defined:

(function () {
  function isEmpty(o)
  {
    for(var key in o)
      if(o.hasOwnProperty(key))
        return false;
    return true;
  }

  function isBrowserIE()
  {
    return navigator.userAgent.toLowerCase().indexOf('msie') > -1;
  }

  var o = {};
  o.prop1 = 5;
  o.prop2 = 10;

  var b = isEmpty(o);
  alert(b);
}());
@michaelficarra

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2015

Top level declarations are observable. They add their names to the global object.

@edwardcoolson

This comment has been minimized.

Copy link
Author

commented Jun 23, 2015

Okay, in our example with all the functions at our fingertips it is quite possible. But in real life I'm using a bunch of js files (may be external). There can be globally defined functions like isBrowserIE() in those files. I use ones and don't use others. In fact, I'm using only a small part of those functions, say 10 percents of the whole lot.

How can I write my code to get rid of those 90% unused functions? With uglifyJS it seems that all that stuff will directly go in my production code. Is it some way to solve this? (Manual removal of unused functions is not a solution for me.)

@edwardcoolson

This comment has been minimized.

Copy link
Author

commented Jun 23, 2015

@Skalman You wrote: "... if Uglify removed the function it would no longer be globally defined". It's exactly what I want! My code decides what to use and what not! There is no other user of the code except me. So if I don't use a function I don't need it at global scope! Please remove it from my production code!

@Skalman

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

@edwardcoolson I also wrote "Try wrapping everything in an IIFE so that the function isn't globally defined". An IIFE (immediately-invoked function expression) is written like this:

(function () {
  ...all your code...
)());
@edwardcoolson

This comment has been minimized.

Copy link
Author

commented Jun 24, 2015

@Skalman So I guess I need these automation steps:

  • concatenate all external js code + my js code that uses it
  • wrap that all inside IIFE
  • minify that IIFE with uglifyJS

Sounds reasonable. Thank you Dan.
But to me, if there was an option in uglifyJS to remove unused global functions it would be more straightforward.

By the way, what automation tool is better for this, Grunt or Gulp?

@avdg

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2015

Both automation tools should be fine, they are practically the same for basic usage.

@edwardcoolson

This comment has been minimized.

Copy link
Author

commented Jun 24, 2015

Thank you guys

@edwardcoolson

This comment has been minimized.

Copy link
Author

commented Jun 25, 2015

@Skalman I wrapped my code inside IIFE -- unfortunately unused functions are still not removed! As an example I have this minimum code that still reproduces the error. As you see, all is inside IIFE but this hapless isBrowserIE() function that I don't use (BTW I don't use any other function in this example) is still there. This is the original code:

(function() {

var JSON = JSON || {};

JSON.stringify = JSON.stringify || function (obj)
{ 
  var t = typeof (obj); 
  if (t != "object" || obj === null) { 

    // simple data type 
    if (t == "string") obj = '"'+obj+'"'; 
    return String(obj); 
  } 
  else { 

    // recurse array or object 
    var n, v, json = [], arr = (obj && obj.constructor == Array); 

    for (n in obj) { 
      v = obj[n]; t = typeof(v); 

      if (t == "string") v = '"'+v+'"'; 
      else if (t == "object" && v !== null) v = JSON.stringify(v); 

      json.push((arr ? "" : '"' + n + '":') + String(v)); 
    } 

    return (arr ? "[" : "{") + String(json) + (arr ? "]" : "}"); 
  } 
}

JSON.parse = JSON.parse || function(str)
{
  //if (str === "") str = '""';

  try {
    eval("var p=" + str + ";");
  }
  catch(e) {
    throw new SyntaxError('JSON.parse');
  }
  return p;
}

function isBrowserIE()
{
  return navigator.userAgent.toLowerCase().indexOf('msie') > -1;
}


function MyObject(a, b)
{
  var a_ = a;
  var b_ = b;

  this.sum = function()
  {
    return a_ + b_;
  }
}

var o = new MyObject(5, 10);
var s = o.sum();

})();

This is minified code:

!function(){function isBrowserIE(){return navigator.userAgent.toLowerCase().indexOf("msie")>-1}function     MyObject(r,t){var n=r,e=t
this.sum=function(){return n+e}}var JSON=JSON||{}
JSON.stringify=JSON.stringify||function(r){var t=typeof r
if("object"!=t||null===r)return"string"==t&&(r='"'+r+'"'),r+""
var n,e,i=[],o=r&&r.constructor==Array
for(n in r)e=r[n],t=typeof e,"string"==t?e='"'+e+'"':"object"==t&&null!==e&&    (e=JSON.stringify(e)),i.push((o?"":'"'+n+'":')+(e+""))
return(o?"[":"{")+(i+"")+(o?"]":"}")},JSON.parse=JSON.parse||function(str){try{eval("var      p="+str+";")}catch(e){throw new SyntaxError("JSON.parse")}return p}
var o=new MyObject(5,10),s=o.sum()}()

So I come to conclusion that uglifyJS does not remove unused functions even inside IIFE.

@Skalman

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

@edwardcoolson, the reason is that you are using eval(). As far as Uglify knows, you might be using isBrowserIE(). Actually, the following snippet would call the function:
JSON.parse('{notReallyJson:isBrowserIE()}')

You have some other bugs in your code too, e.g. var JSON = JSON || {}; doesn't do what you want it to do, because variable declarations are hoisted. That will be interpreted as:

var JSON;
JSON = JSON || {};

You probably mean var JSON = window.JSON || {} or perhaps var JSON = window.JSON = window.JSON || {}.

@edwardcoolson

This comment has been minimized.

Copy link
Author

commented Jun 25, 2015

@Skalman Thank you Dan. The code:

var JSON = JSON || {};

was designed to be used in global scope not inside IIFE. When wrapping into IIFE as you rightly noted it should be replaced for:

var JSON = window.JSON || {};

So, in other words, uglify's requirement to wrap all code into IIFE -- to be able to remove unused functions -- causes serious side effects. On the other hand, not wrapping all code into IIFE does not remove unused functions altogether. So, uglifyJS turns out to be useless for my use case. (Remember, I use bunch of external code that I don't want to put my hands on.)

By the way, the corrected statement

var JSON = window.JSON || {};

if wrapped into IIFE also does not remove isBrowserIE() for modern browsers that have JSON natively supported! The function body with that devil's eval():

JSON.parse = JSON.parse || function(str)
{
  //if (str === "") str = '""';

  try {
    eval("var p=" + str + ";");
  }
  catch(e) {
    throw new SyntaxError('JSON.parse');
  }
  return p;
}

is not defined if JSON.parse() is natively supported! But uglifyJS does not trouble itself to analyze this.

Verdict. To be able to safely & effectively use uglifyJS I need the compress option 'Remove top level unused functions/vars'. Let it be turned off by default, if you are so bothered about top level names availability.

Thank you.

@michaelficarra

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2015

@edwardcoolson You're using a direct call to eval. There's no way we could know that you didn't ever intend to access your isBrowserIE function through that. You should be able to resolve your problem by replacing the direct call to eval with an indirect call to eval: (0, eval)(...).

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

commented Mar 4, 2017

The original code now works with -c toplevel:

$ uglifyjs test.js -mc toplevel
WARN: Dropping unused function isBrowserIE [test.js:9,9]
function isEmpty(r){for(var o in r)if(r.hasOwnProperty(o))return!1;return!0}var o={};o.prop1=5,o.prop2=10;var b=isEmpty(o);alert(b);

@alexlamsl alexlamsl closed this Mar 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.