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

Browserify-specific fix for #29 #36

Merged
merged 9 commits into from
Nov 2, 2016

Conversation

aickin
Copy link
Collaborator

@aickin aickin commented Nov 1, 2016

Fix for #29, trying to optimize browserify modules, in a very specific way.

This fix will optimize function expressions that are:

  • an element of an array,
  • which in turn is a value of an object literal,
  • which has a numeric name in the object literal,
  • where the object literal is an argument to a function call,
  • and the function call is an IIFE.

Added both positive and negative test cases. I did some basic testing against the PouchDB benchmark, and this PR makes it a lot faster.

@nolanlawson
Copy link
Owner

This looks fantastic! One thing I was momentarily confused on was:

browserify-style function call that has non-numeric object indices shouldn't be optimized

I thought this referred to Browserify with the bundle-collapser plugin vs without, but I tested manually and confirmed that both bundle-collapser style and regular style are covered (i.e. parens are correctly added). I'll add some additional tests on the way in to make this clearer.

BTW just a quick remark that these PRs are incredibly thorough and well-written, and I'm extremely grateful that you're improving my little weekend project so much. 🙇 optimize-js is suddenly turning into a very nice general-purpose tool!

@nolanlawson nolanlawson merged commit 0cba85e into nolanlawson:master Nov 2, 2016
nolanlawson added a commit that referenced this pull request Nov 2, 2016
@aickin
Copy link
Collaborator Author

aickin commented Nov 2, 2016

BTW just a quick remark that these PRs are incredibly thorough and well-written

Aw shucks, my pleasure. Thanks for accepting the PRs!

nolanlawson added a commit that referenced this pull request Nov 3, 2016
@nki2
Copy link

nki2 commented Dec 21, 2016

It seems this update is not included in the release v1.0.2 yet. (not avail via npm neither)
Could you please check (and make it available if possible)
Thank you!

PS: I tried it on a large browserify project and I had HUGE improvements!! Great work!

@nolanlawson
Copy link
Owner

I'm going to work on a large rewrite of the benchmarks to include better stats, and then re-run them to ensure there are no regressions. Unfortunately I've been swamped, hence why this has sat for so long.

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.

3 participants