Repository navigation
Add babel-plugin-lodash if app depends on Lodash #1069
Description
Update: issue is taken by @iam4x. Please don't try to "finish it first"!
As seen in Automattic/wp-api-console#45, it seems like a good idea to do.
While we generally don't harcode support for libraries, Lodash is extremely popular to the point it can deserve special treatment, especially given that the plugin is easy to include conditionally and has significant benefits.
Our Babel preset could accept isUsingLodash as an option:
{
"presets": ["react-app", {
"isUsingLodash": true
}]
}It should be false by default.
Inside the preset, if isUsingLodash is true and the environment is production (we already have an if branch for environments there), we should require() and include babel-plugin-lodash (but not in other cases).
In Webpack config, we should conditionally pass isUsingLodash depending on whether app's package.json's dependencies contains Lodash of compatible versions.
Finally, after ejecting people who didn't depend on Lodash should have "presets": ["react-app"] in .babelrc, but people who used Lodash at the time they ejected should see "presets": [["react-app", { isUsingLodash": true }]] in it.
Activity
iam4x commentedon Nov 20, 2016
My turn to contribute, I will have time Tuesday 👍
gaearon commentedon Nov 20, 2016
Sounds great, you got it!
tmc commentedon Nov 20, 2016
This seems like an odd departure from the intentional simplicity of this project.
gaearon commentedon Nov 20, 2016
Simplicity is in how to use it, not necessarily in how it works internally.
I would not even call how it works internally "simple" today.
What specific concerns do you have?
gaearon commentedon Nov 20, 2016
To be clear this change has zero effect on the "public" API pre-ejecting or even post-ejecting if you don't use Lodash. If you do use it, you'll see one line in
.babelrcbeing a few characters longer after ejecting. But it can make a big difference in bundle size and it's a great example of how we can make good choices for our users without them having to worry about it.I just described how the feature should be implemented, not how it is used.
alanhussey commentedon Nov 21, 2016
I haven't checked recently, but at one point
babel-plugin-lodashdid not support lodash's chaining syntax (_(data).filter(...).take(5)– is that a concern here? There could be CRA apps using that syntax which would be broken by upgrading to this.arunoda commentedon Nov 21, 2016
I tend to agree with @tmc
I think this is not directly related to this project. I got @gaearon's concern about bundle size and this is something doesn't affect the public API.
But there's already a fix available for this original issue. Just use the following syntax.
That's what recommends by lodash.
I think there could be more optimizations we could do like this. (may be in the future for different libraries). So, are we going to add those babel plugins as well?
Anyway, I think it's time to re-think about allowing the user to customize babel and webpack config.
fdaciuk commentedon Nov 21, 2016
Just complementing @arunoda's post:
You may even use:
And just install
lodash.pickinstead all lodash.So, it will be great if we had a way to customize webpack witout eject it :)
havenchyk commentedon Nov 21, 2016
@fdaciuk from my perspective it will be a hell to manage a lot of lodash functions with such approach. The method from @arunoda is much better because it allows you just to have a single dependency, bundle size in this case will be small and
package.jsonwill be clean.gaearon commentedon Nov 21, 2016
I know that but many beginners don't.
I would add them for very popular libraries. Let's not forget Lodash is (one of?) the most depended package on npm.
Customising Babel config leads to hard-to-trace issues like https://github.com/cssinjs/react-jss/issues/32#issuecomment-190819245. Until Babel fixes such issues I am opposed to it.
Customising Webpack config is even more dangerous because we plan to migrate to Webpack 2 soon. So there would be breakage if you rely on the configuration format. Not to say changes like #1059 would be hard to introduce without breaking everyone. So no, I don't think this is a good idea for the project either at this time.
If it breaks the code we can't use it of course. My assumption is it doesn't but we need to check the issue tracker. However if by "doesn't work" you mean that it bails out of optimizing this is fine.
julien-f commentedon Nov 21, 2016
👍 but I don't like the
isUsingLodashflag, IMHO,babel-plugin-lodashshould behave correctly iflodashis not used.gaearon commentedon Nov 21, 2016
I would love to hear more arguments for why we shouldn't do it but not the "slippery slope" kind. I think nothing prevents us from being reasonably smart and I'm open to adding more useful plugins if they can be enabled when you use a specific library, don't need configuration, and just make your code run better.
14 remaining items
nylen commentedon Nov 22, 2016
It seems like the above change to babel-plugin-lodash is necessary for create-react-app, unless I am missing something any code using
chainwould result in an error from babel otherwise?iam4x commentedon Nov 22, 2016
I like the option, like @gaearon said lodash is lot used and I think sometimes it's better to take decision for the user (also providing opt-out option with ejecting) and educate him through the creat-react-app choices on how to build webapps.
And about
_.chainmethod these are the thoughts I share: "Why using chain is a mistake"Dropped my first work into a PR, will wait the go to continue from the maintainers after everyone agreed on how to implement this option 👍
gaearon commentedon Dec 5, 2016
The showstopper here is plugin failing when
_.chainis used.But it seems unavoidable because if plugin didn't fail, it would risk users including both a full build and a modular build which is even worse from size point of view.
So we likely won't proceed with it.
jdalton commentedon Dec 5, 2016
I may be able to simply punt on cherry-picking in babel-plugin-lodash if chaining is detected instead of throwing an error. Pull requests welcomed.
gaearon commentedon Dec 5, 2016
The problem is you couldn't do that reliably cross-file, could you? That is, if file A uses
_.chainbut file B uses a regular form, Babel plugin would bring both versions into the bundle.jdalton commentedon Dec 5, 2016
I could set a flag so once chain is hit, it punts for all other files after. So while not perfect it reduces the impact for the edge case and improves things for everyone else.
julien-f commentedon Dec 5, 2016
@jdalton Is it really something that should be done? I thought using chaining was discouraged?
jdalton commentedon Dec 5, 2016
If you dig a more FP approach or cherry-picking then you'll likely avoid chaining. What choice do I have when folks like Dan refuse to use it otherwise 😋 ? From my standpoint, more folks with smaller bundles is a good thing. So if it helps to avoid throwing, it's a win.
gaearon commentedon Dec 5, 2016
To be clear I’m not a fan of chaining myself but I can’t break existing project code and all the snippets people copy-paste from StackOverflow.
gaearon commentedon Dec 5, 2016
Are there plans to drop chain altogether in some future major?
jdalton commentedon Dec 6, 2016
Yep!
In v5 by way of removing the monolithic entry point in favor of
babel-plugin-lodashcherry-picking.gaearon commentedon Feb 24, 2017
Closing because of #1088 (comment).
Happy to revisit some time later again!
CrisLi commentedon Mar 17, 2017
If we upgrade the lodash to v5, can we use babel-plugin-lodash in the next release of create-react-app?