Closed aickin closed 7 years ago
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. :confused: 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?
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.
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. ;) )
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:
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!
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:
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!