apollo-cache-redux icon indicating copy to clipboard operation
apollo-cache-redux copied to clipboard

Import functions from lodash independently

Open lamflam opened this issue 6 years ago • 10 comments

The current package is pulling in all of lodash as a dependency which can result in unnecessarily large bundle sizes. This change imports the functions independently instead.

I've never used typescript before and this syntax seemed a little strange (import ... = require(...)) but I got it from this thread: https://github.com/lodash/lodash/issues/3192.

Happy to make adjustments if needed.

lamflam avatar Apr 13 '18 21:04 lamflam

Hi @lamflam , thanks for the PR. Since we're using only 2 methods from lodash, I'd prefer if we installed lodash.merge and lodash.cloneDeep and have imports like import * as cloneDeep from 'lodash.clonedeep';, if that's okay with you.

Thanks.

rportugal avatar Apr 13 '18 21:04 rportugal

@rportugal I tried to switch to the syntax you mentioned, as well as the other options listed in https://github.com/lodash/lodash/issues/3192, but all of those options give typing errors. I could also change these too const cloneDeep = require('lodash/cloneDeep') if you'd prefer that syntax, but there's currently an eslint error for that syntax as well.

lamflam avatar Apr 17 '18 15:04 lamflam

The trouble with using separate packages for different lodash functions is that if anything else in your app uses lodash, then instead of having just one big problem you have 1 big problem and also many small problems.

In recent versions of webpack and lodash, this problem is fixed in general by a combination of tree shaking and the sideEffects flag in package.json: https://medium.com/webpack/webpack-4-beta-try-it-today-6b1d27d7d7e2

hedgepigdaniel avatar Apr 23 '18 02:04 hedgepigdaniel

Was running into this today myself. Would love to see this resolved soon.

Thanks! Piyush

pachuka avatar Apr 26 '18 03:04 pachuka

@hedgepigdaniel I'm obviously not an expert in Webpack, but would that configuration be on this package or consuming apps?

rportugal avatar Apr 26 '18 05:04 rportugal

Yes, I mean in consuming apps. If one of these two options are satisfied:

  1. All imports to lodash import specific functions via a path inside the lodash package, e.g. import unique from 'lodash/unique';
  2. The consuming app transforms the source code with babel-plugin-lodash (which just transforms the code to option 1)

Then webpack should be able to remove the modules in lodash that are not used by the app. Before webpack 4 this didn't work, because if you imported lodash you imported the lodash entry point file which in turn imported everything else inside lodash. The innovation in webpack 4 is that since lodash has set the sideEffects flag to true in its package.json, webpack can assume that even though lodash/index.js imports the rest of lodash, it can remove everything except the exports that were in turn imported by the user into the app.

Using seperate packages like import unique from 'lodash.unique'; works to prevent the entire lodash library from being included regardless of webpack/lodash version, but it means that in the best case with the latest versions of webpack and lodash and babel set up correctly, if the app or another depended on package uses the real lodash package, then there end up being 2 seperate copies of the code for lodash unique.

More info: https://advancedweb.hu/2017/02/07/treeshaking/

hedgepigdaniel avatar Apr 26 '18 06:04 hedgepigdaniel

@hedgepigdaniel @rportugal I think I am going to have to disagree on this being the responsibility of the consuming app... Take for example a simple library that consumes this library using webpack with just ts-loader. Why would that library or even a basic react app that builds purely with ts-loader without any babel dependencies be required to pull in babel just to get this 3rd party library to bundle efficiently? Also AFAIK the babel plugins only apply to the code in the src folder of the app since typically you exclude node_modules from your loaders, so that transform would only apply to the app source, not any 3rd party source being pulled in. Not only that, but webpack itself does tree-shaking if the libraries being pulled in are written properly.

Also, I don't think your description of sideEffects is quite right, from the documentation: The "sideEffects": false flag in big-module 's package.json indicates that the package's modules have no side effects (on evaluation) and only expose exports. This allows tools like webpack to optimize re-exports.

Note that lodash has it set to False since Dec 2017, which is correct based on the description above. I think this would only apply if this library were re-exporting lodash.

I'm not 100% sure what the correct solution is, I've been trying to determine that myself over the last few weeks as I work on my own library/apps at work. What I do know is that the change proposed in this PR significantly reduces this libraries bundle size in general, and most likely this library should also mark itself as sideEffect: false if it meets the requirements for that and that might help webpack tree-shake this package better.

I should have some time this weekend to fork this and take a look at the bundling myself, so I'll update again (or make a PR) if I find a better solution.

Thanks!

pachuka avatar Apr 26 '18 12:04 pachuka

@pachuka testing sounds good haha

I think this would only apply if this library were re-exporting lodash.

Hmm, I guess it depends on how this library is built - I think that if it is build such that it can be imported with es6 exports and it imports lodash functions with es6 exports, then tree shaking will work (because webpack still parses things in node_modules even if it doesn't put them through babel - its webpack that does the tree shaking rather than babel - babel just needs to not get in the way by changing es6 exports to commonjs exports as is the default.

So to be clear, I don't think you need babel or that babel plugin in a consuming project to have the tree shaking working - you just need:

  • consuming app to import this project with es6 imports
  • this project to use es6 exports
  • this project needs to import lodash functions with es6 imports.
  • consuming project to not have any loader that transpiles es6 exports to commonjs before webpack does its tree shaking (e.g. if it has babel-loader, it needs presets: ["env", {"modules": false }])
  • webpack 4+
  • recent lodash with sideEffects: true in package.json

I don't meant to get in the way of progress - to be clear:

Changing imports to import fn from 'lodash/fn'; as you have done in this PR doesn't cause any issues afaik. For people who have set up recent webpack, babel, and lodash so that tree shaking and the sideEffects flag work, it should make no difference. For people who have used that babel plugin, it should make no difference. For people who have not set up webpack such that tree shaking works (lots of people), it will make a big improvement if this is the only library they have used that imports this version of lodash.

What I want to point out is that using import fn from 'lodash.fn'; means that the first two groups of people (tree shaking + sideEffects flag working or using the babel-plugin) they may end up with two copies of fn - one from lodash/fn, and one from lodash.fn. That might still be a good compromise - I just want to point it out.

hedgepigdaniel avatar Apr 27 '18 00:04 hedgepigdaniel

@rportugal Since the current changes I'm suggesting don't seem to have any downsides, are you open to merging this and possibly making additional changes later if you find a better approach?

lamflam avatar May 16 '18 15:05 lamflam

@rportugal any updates on this? This would be super helpful for our team to reduce our bundle size.

emroussel avatar Nov 28 '18 16:11 emroussel