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

Not as optimized for Webpack as it could be #7

Closed
nolanlawson opened this issue Sep 19, 2016 · 13 comments
Closed

Not as optimized for Webpack as it could be #7

nolanlawson opened this issue Sep 19, 2016 · 13 comments

Comments

@nolanlawson
Copy link
Owner

Looking at the Webpack output, it appears to me that those modules that are passed in as an array to another function (and are therefore not wrapped) should probably be wrapped. As with Browserify, I'd wager it's rare for a module not to be immediately require()d, meaning that wrapping all of them would provide a perf boost.

Need a reasonably-sized Webpack bundle in order to test this and confirm, though. TODO: find some large-ish library that is built with Webpack.

@nolanlawson
Copy link
Owner Author

@ggoodman
Copy link

@TheLarkInn
Copy link

Did we test these w/ the V8LazyParseWebpackPlugin?

@ggoodman
Copy link

@TheLarkInn if I had some guidance on moving to Webpack 2, I would try it out.

@TheLarkInn
Copy link

That plugin may work with webpack 1 also, but what specifically do you need help with for webpack 2.

@nolanlawson
Copy link
Owner Author

This may be a good first patch, given that you basically just need to add a new test case (input.js and output.js) and then add a new condition to check if the function is inside an array inside of a call expression. ASTExplorer is very helpful here.

@vigneshshanmugam
Copy link
Contributor

@TheLarkInn Does lazyparseplugin adds the parans for the modules in array?

@TheLarkInn
Copy link

It sure does!! If you have a large app feel free to try it. Only catch is that I believe you have to tell UglifyJsPlugin compress option to set negate_iife: false.

@TheLarkInn
Copy link

@vigneshshanmugam if you have an app that we can test with I'd be interested to see the statistics.

@vigneshshanmugam
Copy link
Contributor

vigneshshanmugam commented Oct 1, 2016

@TheLarkInn I tried it and updated the gist. It fails to wrap the inner functions as well.

Also updated the readme here - TheLarkInn/LazyParseWebpackPlugin#4

@TheLarkInn
Copy link

Awesome!!! I intentionally didn't wrap the inner functions inside modules because I wasn't sure the benefit, but it's definitely possible to use a webpack parser plugin to do this.

@vigneshshanmugam
Copy link
Contributor

Ahh got it.. But i believe the benchmarks in the README includes the wrapping for inner functions as well.. Anyways as you said, its possible to do this :)

aickin added a commit to aickin/optimize-js that referenced this issue Oct 24, 2016
…dds ability to wrap functions that are elements of an array literal in an argument list
nolanlawson pushed a commit that referenced this issue Nov 1, 2016
* Fix for #7: Not as optimized for Webpack as it could be. Adds ability to wrap functions that are elements of an array literal in an argument list

* Added a few negative case tests in response to code review.

* Modified the array literal logic to very specifically track with webpack patterns rather than all array literal function parameters.
@nolanlawson
Copy link
Owner Author

Fixed in #33

nolanlawson pushed a commit that referenced this issue Nov 2, 2016
* Fix for #7: Not as optimized for Webpack as it could be. Adds ability to wrap functions that are elements of an array literal in an argument list

* Added a few negative case tests in response to code review.

* Modified the array literal logic to very specifically track with webpack patterns rather than all array literal function parameters.

* Added optimization of browserify modules.

* Refactor of browserify and webpack logic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants