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

Webpack-specific fix for issue #7 #33

Merged
merged 4 commits into from
Nov 1, 2016

Conversation

aickin
Copy link
Collaborator

@aickin aickin commented Oct 29, 2016

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!

…dds ability to wrap functions that are elements of an array literal in an argument list
…ack patterns rather than all array literal function parameters.
@nolanlawson
Copy link
Owner

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
Copy link
Collaborator Author

aickin commented Oct 31, 2016

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
Copy link
Collaborator Author

aickin commented Oct 31, 2016

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. ;) )

// estree-walker does not follow the link.
node.parent = function () {
return parent
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

node.parent().parent() && // CallExpression
node.parent().parent().callee && // function that is being called
node.parent().parent().callee.type === 'FunctionExpression'
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love the name of this function! 😂

Copy link
Owner

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 nolanlawson merged commit cd00deb into nolanlawson:master Nov 1, 2016
@nolanlawson
Copy link
Owner

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
Copy link
Collaborator Author

aickin commented Nov 1, 2016

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants