-
Notifications
You must be signed in to change notification settings - Fork 383
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
Add babel plugin lodash #232
Add babel plugin lodash #232
Conversation
This should help reduce the footprint of lodash dependency.
@jdalton I am not specifically attempting to troubleshoot that plugin here, no -- this PR just arose out of some attempts on my part to a) understand why the js bundles generated by react_on_rails seemed so large, and b) follow some suggestions to reduce those sizes by using cheers |
The bindall issue is new afaik, ill look into it. As for the size increasing, are multiple versions of lodash getting imported now (e.g. from a dep module?) |
@megawac multiple versions of lodash would be my guess, yes, I'll see if I can track that down. |
Last two commits should a) fix specs, b) fix linter complaint, c) address increased file size in dev build. @megawac I believe you were right -- one of my changes was incorrectly importing To the maintainers -- I'd be happy to squash these junk commits later, if/when you'd want to merge this. |
@lukeasrodgers Absolutely awesome! Let's squash to one commit. If you can write up a few notes in the docs directory, that would be a bonus! I actually want to reconcile the docs between here and react_on_rails/docs eventually. The very best option doc wise is to add a note in the README.md and to open a PR in react_on_rails. Naturally, we'd like to have most of these changes eventually make it to the app generator in react_on_rails, but our priorities lie in:
Thanks again! |
@justin808 happy to try to write some documentation
I will squash after we've finalized the docs. cheers |
@lukeasrodgers Let's summarize these recommendations in a doc in the react_on_rails repo and we can link to this PR for more details. We should name the doc: /docs/additional_reading/minimizing_bundle_size.md If you click to edit your comments with images, you can copy the markdown that creates the image. I think that is helpful to show the benefit. I'd recommend that we put a code comment that mentions the importance of using the import { bindAll } from 'lodash'; rather than import _ from 'lodash' Just squash, put a little bit of docs, and if we need further revisions, no worries. Thanks again so much! |
BTW if you don't like unscoped // libs/lodash.js
import { bindAll, ... } from 'lodash';
export default { bindAll, ... };
// some.js
import _ from 'libs/lodash';
_.bindAll(...); |
@alexfedoseev @lukeasrodgers @jdalton I really like Alex's idea for 2 reasons:
I'd vote for going with @alexfedoseev recommendation and calling the file:
and a file will have import _ from 'libs/slim_lodash.js';
_.bindAll(...); Agree? |
@alexfedoseev and @justin808 sounds good to me, your repo, so your call. One potential drawback of that approach (afaict) is that now if I want to use a new function from lodash I have to change two files (the lib and my own file) instead of just changing my own and letting the lodash plugin sort it out. But I don't think it matters too much either way. |
👍 |
This helps ensure standardized and optimizated usage of lodash functionality across the application.
Okay, the last commit attempts to implement the lib functionality discussed above. Oddly, the lodash import syntax that worked previously broke when trying this approach -- but only when compiling a production build (caused the functions to be undefined). The syntax in this commit appears to work for both dev and prod builds. Also, surprisingly, this change alone appears to have shaved ~1 MB off the dev app bundle, down to 4.5 from 5.6 Slightly OT: is webpack + babel always this much of a black box? I'm used to "run these files through closure compiler; adding a new file to that list will make the generated code slightly larger." I get that much more is going on here, but am just wondering whether this "make some minor change and all of a sudden your generated code is 1MB smaller" experience is typical for this kind of webpack+babel pipeline. |
I just reread the thread:
Thats an erroneously formed import. You want |
@@ -0,0 +1,3 @@ | |||
import bindAll from 'lodash/bindAll'; | |||
import find from 'lodash/find'; | |||
export default { bindAll, find }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Does this syntax seem correct to you?
import { bindAll, find } from 'lodash';
export default { bindAll, find };
This seems correct to me, and works fine on the dev build, but breaks the production build on this repo: Uncaught ReferenceError: bindAll is not defined
. I unfortunately don't have enough experience with these technologies to be able to determine why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I can reproduce though. Looking into it
The issue is that babel-plugin-lodash does not recognize/support export specifiers; I don't plan to support it either, when using the babel plugin you should use lodash as normal and let the plugin handle the grunt work of importing the modules directly. If you used the methods directly rather than building the slim_lodash
file, babel-plugin-lodash would work as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous discussion on the PR. The idea is to avoid including all of lodash when only specific functionality is required. I appreciate you taking time to look into this, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying @megawac.
@justin808 @alexfedoseev I'm going to close this PR. I'm not the right person to try to implement this functionality, I don't have enough experience with webpack/babel/es6 modules, and I think my efforts are just causing more confusion and wasting reviewers' time. I suspect it will be easy for someone else with more experience.
cheers
Attempts to address #204
I am new to babel and webpack, so I may have screwed some obvious things up here.
See http:https://forum.shakacode.com/t/generated-bundle-size/491/4 for some relevant back and forth with @justin808
Below are some before and after screenshots. Note that to get the "prod" packages, I edited
lib/assets.rake
to explicitly use a prod build, which it didn't seem to be doing:Curiously, with the lodash plugin, the dev app bundle actually increases in size by 300kb, though with the prod build it goes down by 400kb.
Also the syntax mentioned by @jdalton on #206 (
import { bindAll } from 'lodash/bindAll'
) did not work, and appeared to causebindAll
to beundefined
.dev before
![react-webpack-rails-tutorial-dev](https://cloud.githubusercontent.com/assets/816444/13114153/3a42355e-d560-11e5-9815-e815662e1635.png)
dev after
![react-webpack-rails-tutorial-dev-lodash-plugin](https://cloud.githubusercontent.com/assets/816444/13114151/3a40ba4e-d560-11e5-86e2-3c446a6635b2.png)
prod before
![react-webpack-rails-tutorial-rake-precompileprod](https://cloud.githubusercontent.com/assets/816444/13114152/3a420ac0-d560-11e5-8814-206ce0ada74e.png)
prod after
![react-webpack-rails-tutorial-rake-precompileprod-lodash-plugin](https://cloud.githubusercontent.com/assets/816444/13114150/3a4044ba-d560-11e5-94d6-121f97a747a6.png)