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

Don't include all of lodash #131

Closed
dmethvin-gov opened this issue Jul 2, 2018 · 10 comments
Closed

Don't include all of lodash #131

dmethvin-gov opened this issue Jul 2, 2018 · 10 comments
Assignees
Labels
[practice] engineering Engineering related work [type] debt Tech debt, refactors, maintenance issues

Comments

@dmethvin-gov
Copy link
Contributor

Currently, many files use the syntax import _ from 'lodash/fp' to pull in all of lodash, which tends to be a very big file. Instead they should pull in the specific functions they need via something like import get from 'lodash/fp/get'. That allows tools like webpack to pull in only the needed code.

However, there are also lodash-like functions being defined in https://github.com/usds/us-forms-system/tree/2410134ffda3767058ae7ef16e512e3fc5124632/src/js/utilities/data which may be better to use if they're already in this lib for other reasons. Part of this ticket would be to investigate why that's the case--should we be dropping lodash or maybe not using these custom functions? The vets.gov repo may have more history.

@dmethvin-gov dmethvin-gov added [type] enhancement New feature or request [practice] engineering Engineering related work [type] debt Tech debt, refactors, maintenance issues labels Jul 2, 2018
@jbalboni
Copy link
Contributor

I wrote this the other day in the vets.gov slack, so hopefully it's useful context:

The history of our Lodash approach is:

  1. We were using lodash for some things when HCA was the only form
  2. We started using lodash/fp when we were building the first non-HCA form because it made code in reducers a little easier to write and was a smaller jump than using something like ImmutableJS
  3. When we did a bunch of form system refactoring last December, we started creating some smaller clones of the commonly used lodash/fp functions because lodash is a big dependency and it would be nice to get rid of it.

The vets.gov project (and I would guess the sample app) uses a Babel plugin to automatically convert lodash imports to specific functions like you mention, so you won't save much by converting them manually. But what we've noticed is that importing specific lodash functions is much more efficient than importing specific lodash/fp functions.

The way to save the most space around Lodash is I think to move to custom functions like in utilities/data for the commonly used lodash/fp functions that change data, and try to avoid lodash/fp elsewhere.

@dmethvin-gov
Copy link
Contributor Author

Thanks @jbalboni! At the moment our starter app doesn't use any codemods to reduce the bundle so that might explain some of the difference in size. I've used lodash for a while and without intellisense in the editor or just reading the code it's super confusing because the lodash-fp arg order is different from lodash which I've used a lot more often. 😵

At this point I've switched to plain old replacement and destructuring in most work which AFAIK is pretty efficient and besides we've already "paid for it" by using Babel. Something like Immer looks interesting because it lets you pretend that the state object is mutable but generates a minimally changed new state from an old state.

@jbalboni
Copy link
Contributor

Yeah, I'm not nearly as much of a fan of lodash/fp as I used to be, since it's confusing alongside regular lodash. But lodash/fp's set and merge are useful when you're trying to update nested data in way that avoids mutation, which is pretty difficult with spread and destructuring.

Immer looks interesting, too, I'll have to look at that more closely.

@jcmeloni-usds
Copy link
Contributor

@dmethvin-gov Is this still an issue? I thought I saw something recently that un-lodashed us, but I might have made that up!

@dmethvin-gov
Copy link
Contributor Author

You're probably thinking of #258 that un-momented us. 😸 We still need to un-lodash. @jbalboni suggested a few options above that we need to explore.

@jcmeloni-usds
Copy link
Contributor

@dmethvin-gov You are correct :)

@jcmeloni-usds jcmeloni-usds removed the [type] enhancement New feature or request label Sep 19, 2018
@jcmeloni-usds jcmeloni-usds added this to the End of Phase 1 milestone Sep 19, 2018
@jcmeloni-usds jcmeloni-usds removed this from the End of Phase 1 milestone Oct 2, 2018
@dmethvin-gov
Copy link
Contributor Author

I looked into this and we are already using babel-plugin-lodash which is supposed to transform the monolithic import _ from "lodash/fp" into imports of specific functions being used in the file. Either it's not working or we are actually using a lot of lodash.

@dmethvin-gov dmethvin-gov self-assigned this Oct 5, 2018
@dmethvin-gov
Copy link
Contributor Author

I will try experimenting with lodash-webpack-plugin but that will need to be done in the starter app.

@jbalboni
Copy link
Contributor

jbalboni commented Oct 5, 2018

The plugin is working, you can see it in the built output: https://github.com/usds/us-forms-system/blob/master/lib/js/state/reducers.js#L7 (the source for that file imports lodash/fp), it's just that importing specific lodash/fp functions is far less efficient than regular lodash. For example, if you compare a bundle that just imports lodash/fp/uniqueId vs lodash/uniqueId, with no other code, the fp version is 47kb and the regular version is 2.65kb.

We also experimented with lodash-webpack-plugin, but decided that it was confusing to keep track of what features of lodash we had turned on or off. It also wasn't necessarily a huge savings (turning of currying I think was the biggest reduction, but we also used that in places).

@dmethvin-gov
Copy link
Contributor Author

It looks like @jbalboni is right, this isn't all of lodash but just what we are using, which is kind of sad given how much code it is. I agree as well that lodash-webpack-plugin isn't that convenient to configure, especially with a project under development as the dependencies may be changing often. I'll close this ticket but there's plenty left to do in #152.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[practice] engineering Engineering related work [type] debt Tech debt, refactors, maintenance issues
Projects
None yet
Development

No branches or pull requests

3 participants