Webpack-specific fix for issue #7

[aickin]

This is a more webpack-specific fix for issue #7 than the solution presented in PR #28. Rather than optimize every function expression that is an element of an array literal param, this PR only optimizes function expressions that are elements of an array literal param of a function expression call.

So, unlike PR #28, this PR will not transform this code:

foo([
  function(o,r,t){
    console.log("element 0");
  }
]);

because foo is not a function expression.

I think that this version is probably better than PR #28, but I'm happy to hear dissenting views!

[nolanlawson]

I tend to agree that this is a better fix! I prefer targeted solutions that look for existing patterns (e.g. Webpack) because that increases the likelihood that we will add parentheses where it really matters, and not just willy-nilly for any function in the code. I'd be happy to merge this in favor of #28.

BTW I have merged #31 because I wanted to get ES6 support in, but unfortunately it seems to cause merge conflicts with the other PRs. 😕 So perhaps I was too hasty to merge. What do you think is the best course moving forward? Should I have chosen another es-walker library or would it not be too much work to merge your fixes into the new master?

[aickin]

Glancing at the changes, I think it shouldn't be too hard to merge in, but those are famous last words. The key is going to be if estree-walker and walk-ast produce significantly different ASTs in any real way.

Let me give it a stab.

[aickin]

Updated!

The only wacky part was that estree-walker doesn't automatically add a pointer to parentNode like walk-ast does. I tried to just set node.parentNode = parent as the first line of enter, but that led to an infinite loop. This was because estree-walker assumes that any node property with an object value is in fact a child, so adding a parentNode property led to infinite loops in the AST. The solution (which is a little hacky?) was to make the link to the node's parent into a function.

All tests pass, including several new negative test cases, and coverage is still 100%.

(And as an added enticement to merge, I have a branch on top of this waiting in the wings which implements a browserify-specific solution to #29. ;) )

[nolanlawson]

agree this is a weird hack, but at least you marked it as such and added a lot of explanation :)

[nolanlawson]

love the name of this function! 😂

[nolanlawson]

@aickin you're a legend; this is a great PR and I especially love the new test cases. I was worried that the change to estree-walker would add a lot of headaches, but it seems we're all good for the most part. Very happy to merge this.

[nolanlawson]

BTW @aickin I've gone ahead and made you a contributor to optimize-js so you have the power to merge PRs, commit to master, etc. My typical policy is to wait for at least one other contributor to +1 a PR, but if you don't get a response within 24 hours or so feel free to timebomb and merge it yourself.

Your Webpack-specific fixes are awesome, and what I'd like to do next is:

• merge the Browserify-specific solution you mentioned :)
• release a new version to npm
• re-run the benchmark and also update the README so that the benchmark results are an accurate representation of the new release. I'm not picky about who runs the benchmarks and on which machine, but an update to the README should probably be offered as a PR so we can at least compare the before-and-after to make sure we didn't regress anywhere. If you don't have time I am very happy to run the benchmarks myself; I'll have some time this weekend because I'll be on a long flight for a conference.

[aickin]

Awesome, thanks for both the invite and the clear guidance on how it is to be used!

I've posted the browserify fix as PR #36. Have at it!

As for the benchmark, I agree that it should be re-run, but I have some concerns that it may not be entirely fair to unoptimized code. I wrote those concerns up as issue #37. Let me know what you think.

Onwards!