Add babel-plugin-lodash if app depends on Lodash

[gaearon]

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.

Update: issue is taken by @iam4x. Please don't try to "finish it first"!

[iam4x]

My turn to contribute, I will have time Tuesday 👍

[gaearon]

Sounds great, you got it!

[tmc]

This seems like an odd departure from the intentional simplicity of this project.

[gaearon]

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]

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 .babelrc being 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]

I haven't checked recently, but at one point babel-plugin-lodash did 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]

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.

import pick from 'lodash/pick';

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]

Just complementing @arunoda's post:
You may even use:

import pick from 'lodash.pick'

And just install lodash.pick instead all lodash.

So, it will be great if we had a way to customize webpack witout eject it :)

[havenchyk]

@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.json will be clean.

[gaearon]

But there's already a fix available for this original issue. Just use the following syntax.

I know that but many beginners don't.

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?

I would add them for very popular libraries. Let's not forget Lodash is (one of?) the most depended package on npm.

I think it's time to re-think about allowing the user to customize babel and webpack config.

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.

but at one point babel-plugin-lodash did not support lodash's chaining syntax

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]

👍 but I don't like the isUsingLodash flag, IMHO, babel-plugin-lodash should behave correctly if lodash is not used.

[gaearon]

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.

[gaearon]

IMHO, babel-plugin-lodash should behave correctly if lodash is not used.

I didn't mean it would behave incorrectly, I meant that we should optimize performance and avoid running it for people who don't depend on Lodash.

Now that I think of it, we don't need a flag at all. We can just add a plugin separately.

[julien-f]

I meant that we should optimize performance and avoid running it for people who don't depend on Lodash.

I understand :)

But, AFAIK, babel-plugin-lodash fails when lodash is not installed, even if never used.
Which won't be an issue for you if you conditionally enable it ;)

[nylen]

So, I suppose I started this round of this discussion by hacking our webpack config in Automattic/wp-api-console#45.

If lodash chaining is a problem (#1069 (comment)), this would be a reason not to add babel-plugin-lodash to all of CRA.

If babel-plugin-lodash is added to CRA, we will drop our custom script. But I can't promise that it won't come back again for something else - looking through this project's past issues, there are several tasks that seem like reasonable things for individual projects to do, but are unsupported by CRA for one reason or another.

It's a shame to have to eject, and thereby lose out on all of the future features of create-react-app, because of a couple specific things your project would benefit from.

I think monkey-patching the config like this is a fair compromise, but it comes along with a few extra tasks that should be done:

• Pin to a specific version of react-scripts
• Code defensively: ensure that the objects you're manipulating look as you expect them to
• Whenever you upgrade CRA, test EVERYTHING that's affected by your custom config
• If the size of your custom config keeps growing, reconsider ejecting
• Always keep in mind the GREAT BIG CAVEAT that your custom configuration is unsupported by CRA, you may end up visiting fractals of rabbit holes of bugs like https://github.com/cssinjs/react-jss/issues/32#issuecomment-190819245, etc

I would love to see this technique described in the documentation with some or all of those caveats next to it.

[gaearon]

But, AFAIK, babel-plugin-lodash fails when lodash is not installed, even if never used.

Why does it? This sounds like a bug or a misunderstanding to me. Since Babel operates on files separately, how can this be true?

[gaearon]

But I can't promise that it won't come back again for something else

Fair enough, I'm just asking you to file issues along the way so that excluding them is a conscious decision on our side and not just an omission.

[julien-f]

Why does it? This sounds like a bug or a misunderstanding to me. Since Babel operates on files separately, how can this be true?

@jdalton explained this in this comment.

[glifchits]

@alanhussey's comment above bears discussion.

The _(thing) or _.chain(thing) lodash method is not available, and according to this article that @jdalton mentioned in the babel-plugin-lodash repo, it will not be supported.

If this plugin is disabled except on production builds, then their prod build will throw errors whenever _.chain is used. This seems like a very confusing thing if the user had no idea that create-react-app was automatically using this plugin.

[gaearon]

If this plugin is disabled except on production builds, then their prod build will throw errors whenever _.chain is used. This seems like a very confusing thing if the user had no idea that create-react-app was automatically using this plugin.

Yes, of course if this is how this plugin behaves, we can't include it.
I was assuming it's more .. unobservable.

[gaearon]

@julien-f

Is this a problem though? My suggestion is to include it automatically if lodash is a dependency. So if it's not a dependency we wouldn't include it at all.

[julien-f]

@gaearon, nope, as I said it's not an issue for you :)

[gaearon]

Ah, I missed that sentence, sorry 😅

[jdalton]

@julien-f

Why does it? This sounds like a bug or a misunderstanding to me. Since Babel operates on files separately, how can this be true?

@jdalton explained this in this comment.

I can make it avoid erroring if that helps.

[julien-f]

@jdalton Not necessary for create-react-app, but if it's easy I would appreciate it for myself :)

[jdalton]

Not necessary for create-react-app, but if it's easy I would appreciate it for myself :)

Easy peasy lodash/babel-plugin-lodash@33e503a.

[nylen]

It seems like the above change to babel-plugin-lodash is necessary for create-react-app, unless I am missing something any code using chain would result in an error from babel otherwise?

[iam4x]

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 _.chain method 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]

The showstopper here is plugin failing when _.chain is 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]

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]

The problem is you couldn't do that reliably cross-file, could you? That is, if file A uses _.chain but file B uses a regular form, Babel plugin would bring both versions into the bundle.

[jdalton]

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]

@jdalton Is it really something that should be done? I thought using chaining was discouraged?

[jdalton]

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]

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]

Are there plans to drop chain altogether in some future major?

[jdalton]

Are there plans to drop chain altogether in some future major?

Yep!

In v5 by way of removing the monolithic entry point in favor of babel-plugin-lodash cherry-picking.

[gaearon]

Closing because of #1088 (comment).
Happy to revisit some time later again!

[CrisLi]

If we upgrade the lodash to v5, can we use babel-plugin-lodash in the next release of create-react-app?