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

use main lodash instead of separate packages per function #7

Merged
merged 3 commits into from
Feb 26, 2020

Conversation

gazs
Copy link

@gazs gazs commented Oct 3, 2017

One of our low-hanging fruits is the duplication of lodash imports. Our codebase currently has 4 ways and I'm trying to get it down to 2 (see https://github.com/conde-nast-international/voguede-autopilot/pull/823 ). This is an attempt to fix the upstream instead of trying to rewrite everything on our end.

Copy link

@pgoldrbx pgoldrbx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this lgtm

@rufus2021 who will remember some things I think

cc @CondeNast/autopilot

package.json Outdated
@@ -22,8 +22,7 @@
"license": "MIT",
"dependencies": {
"jsonml.js": "^0.1.0",
"lodash.camelcase": "^4.1.1",
"lodash.isfunction": "^3.0.8"
"lodash": "^4.17.4"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably fine, though we could just specify ^4 to allow maximum deduping downstream?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giles-v
Copy link

giles-v commented Oct 3, 2017

So one of the nice things about this lib is the small size. You're probably juicing it by 15kB or something with this change. Is the assumption that brand apps will always be using the full lodash?

@pgoldrbx
Copy link

pgoldrbx commented Oct 3, 2017

@giles-v right - the trouble is we either need to always use only the exact lodash packages needed, or we need to always use the full, everywhere, so that we can dedupe in the bundle. If we have a combination, the result will always be larger.

I think there's another thread somewhere about this

@brokentone
Copy link

This is excellent, thank you @gazs -- thought I can't see the referenced repo.

@giles-v -- yes I think we have to make this assumption at this point -- and theoretically, it still could be treeshaken downstream. With different packages, it cannot be.

@gazs
Copy link
Author

gazs commented Oct 3, 2017

@giles-v this would only increase the build size for brand apps that didn't use lodash at all or were only using the lodash.foo named modules. If they were using lodash/foo, this should be a size reduction for them, if they are importing the monolithical lodash, this should be a size-neutral change (getting rid of lodash.foo and the other lodash modules it brings in )

Copy link

@jsocol jsocol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see a before/after bundle size comparison from a couple of existing brands before making this switch.

Marking this as "requesting changes" because we can get this data, it's not something we have to guess about. I haven't looked into the code and I'll dismiss this when we know the numbers.

@rufus2021
Copy link

could this could result in two versions of the same method for some apps?

@rharriso
Copy link

rharriso commented Oct 3, 2017

@rufus2021 it could, but even if it isn't resolved by npm, you can force a single version of lodash with webpack

@mize
Copy link

mize commented Oct 3, 2017

Aside from James' timing question, would peerDependencies be useful here? I know we've used the trick in other libs of adding a package to both peerDependencies (when used by a dependent lib/app) and devDependencies (when used for this lib's local dev).

@pgoldrbx
Copy link

pgoldrbx commented Oct 3, 2017

Yeah, I've considered peerDependencies as a solution as well

Copy link

@brokentone brokentone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement which I think we should take at this point.

Copy link

@Juan-M Juan-M left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, 👍 too.

@pgoldrbx
Copy link

Still blocked on the bundle size change?

@pgoldrbx
Copy link

This got lost a long time ago, but really it should have landed. Let's make it land!

@pgoldrbx pgoldrbx merged commit c07e019 into CondeNast:master Feb 26, 2020
@pgoldrbx pgoldrbx mentioned this pull request Feb 26, 2020
@rharriso
Copy link

👍

Looks like I'm still part of some team. Which is good, I'm planning on using this in a project coming up here.

@diffcunha
Copy link

Good to know this is still being used! 👍

@ajkim
Copy link

ajkim commented Feb 26, 2020 via email

@mf2492
Copy link

mf2492 commented Feb 26, 2020

💯

@JulieBusch
Copy link

JulieBusch commented Feb 26, 2020 via email

@bencmilton
Copy link

giphy

@gkilian
Copy link

gkilian commented Feb 26, 2020

Nothing like a Conde reunion inside a PR!!!

@bmullan91
Copy link

Good lad Phil 💪

@brokentone
Copy link

I thought more lodash was always better. E.g. at Condé I made a brute force object property extractor leveraging lodash! https://gist.github.com/brokentone/9639f8e0a073aec66ae3b3c67cdf5d9a

@taveras
Copy link

taveras commented Mar 2, 2020

👏 👏

@Juan-M
Copy link

Juan-M commented Mar 2, 2020

Wait for me!!!

@poscar
Copy link

poscar commented Mar 3, 2020

👋

@Georgette
Copy link

Georgette commented Mar 3, 2020

GeGe was here.

@Georgette
Copy link

ngmz0

@fesebuv
Copy link

fesebuv commented Mar 4, 2020

✅ Well done sir! @pgoldrbx

@whyisjake
Copy link

Well done, everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.