rspivak / slimit

SlimIt - a JavaScript minifier/parser in Python
MIT License
551 stars 94 forks source link

mangler ignores catch() var #7

Closed wkornewald closed 13 years ago

wkornewald commented 13 years ago

When I try to mangle the following code:

function a() {
  var $exc = null;
  try {
    lala();
  } catch($exc) {
    if ($exc.__name__ == 'hi') {
      return 'bam';
    }
  }
  return 'bum';
}

I get the following result:

function a() {
  var a = null;
  try {
    lala();
  } catch ($exc) {
    if (a.__name__ == 'hi') {
      return 'bam';
    }
  }
  return 'bum';
}

As you can see, the $exc variable in the catch() clause was just ignored.

wkornewald commented 13 years ago

I've implemented a patch here: https://github.com/wkornewald/slimit/commit/95afeb88a5266800d22d66fadb657f8dedcb78c9 However, I'm not sure if this is the full solution as I'm already hitting the next bug.

rspivak commented 13 years ago

Hi Waldemar,

I've done some changes very similar to your patch. Could you try the latest 'develop' branch and let me know if there are any other issues. This is how to bootstrap the project:

$ git clone git://github.com/rspivak/slimit.git
$ cd slimit
$ python bootstrap.py -d
$ bin/buildout
$ bin/slimit -m < pyjslib.js > pyjslib.min.js
wkornewald commented 13 years ago

I think you've pushed those changes to the master branch. Anyway, there is still some bug and I'm trying to hunt it down right now. For some reason there is a conflict between mangled names and currently I can only reproduce it with our whole project's source, but not with a simple snippet.

rspivak commented 13 years ago

You're right. I put it mistakenly on a master branch. Good luck with the bug hunting and let me know if I can help out in any way.

P.S. I put changes on develop branch and reset master to its previous state.

wkornewald commented 13 years ago

I think I've found all bugs. At least, now the whole pyjs-generated code works fine. I've also added a unit test for the other bug and fixed the implementation and the unit test for this issue (see my updated pull request).

rspivak commented 13 years ago

Thanks!

I'll review the changes tomorrow and if everything's alright I'll merge them

rspivak commented 13 years ago

Merged. Thanks a lot for the patch.