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

fix: remove lodash-es #2938

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: remove lodash-es #2938

wants to merge 3 commits into from

Conversation

maddhruv
Copy link
Collaborator

@maddhruv maddhruv commented Dec 1, 2020

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Dec 1, 2020

🦋 Changeset detected

Latest commit: bf9d8f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
formik Minor
formik-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Dec 1, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/formium/formik/1oufpw9nf
✅ Preview: https://formik-git-fix-lodash-duplicates.formium.now.sh

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2020

Size Change: 0 B

Total Size: 39.7 kB

ℹ️ View Unchanged
Filename Size Change
packages/formik-native/dist/formik-native.cjs.development.js 306 B 0 B
packages/formik-native/dist/formik-native.cjs.production.min.js 242 B 0 B
packages/formik-native/dist/formik-native.esm.js 238 B 0 B
packages/formik-native/dist/index.js 150 B 0 B
packages/formik/dist/formik.cjs.development.js 15.9 kB 0 B
packages/formik/dist/formik.cjs.production.min.js 6.96 kB 0 B
packages/formik/dist/formik.esm.js 15.8 kB 0 B
packages/formik/dist/index.js 143 B 0 B

compressed-size-action

@TrySound
Copy link
Collaborator

TrySound commented Dec 1, 2020

Maybe better completely get rid from lodash?

@TrySound
Copy link
Collaborator

TrySound commented Dec 1, 2020

Btw does it play with tsdx?

@johnrom
Copy link
Collaborator

johnrom commented Dec 1, 2020

To test the impact of a change like this we should test the impact of file size on the following for both a CJS (where applicable) and ESM project:

  • impact on a webpack project that only imports formik and react
  • impact on a webpack project that imports formik, react and lodash
  • impact on a webpack project that imports formik, react, and lodash-es
  • impact on a webpack project that imports formik, react, and lodash and uses the webpack lodash plugin
  • impact on a webpack project that imports formik, react, and lodash-es and uses the webpack lodash plugin
  • impact on a webpack project that imports formik, react, and lodash and uses the alias technique below
  • impact on a webpack project that imports formik, react, and lodash-es and uses the alias technique in reverse
  • impact on a tsdx project that uses formik, react, and lodash
// webpack.config.js or whatever
webpackConfig.resolve.alias = {
  lodash: 'lodash-es',
};

#2602 (comment)

My theory is that we go ahead and use lodash because it is the standard and users are far more likely to have imported it than lodash-es, reducing the chance of duplication. However, the caveat is that we should change all of our imports within Formik to use the following format:

import join from 'lodash/join'; // or 
import { join } from 'lodash/join';

I think if we combine those two changes, and instruct users to use the webpack lodash plugin, that may be the silver bullet solution to solving this issue in userland for v3. But we'll want to test and make sure webpack lodash plugin can correctly alias lodash/join type imports first.

Basically, we should provide the standard and give devs the opportunity to optimize their project, instead of pre-optimizing on behalf of some, and ultimately hurting performance for others.

Evidence that lodash is more popular than lodash-es: https://www.npmtrends.com/lodash-vs-lodash-es

@johnrom
Copy link
Collaborator

johnrom commented Dec 1, 2020

(we also may get rid of lodash completely depending on the API of v3)

@maddhruv
Copy link
Collaborator Author

maddhruv commented Dec 1, 2020

@johnrom I was just trying formik in a webpack project and was seeing both lodash being included in the final bundle. Not much impact on bundle size, as were named imported but no sense of having duplicate codes.

@johnrom
Copy link
Collaborator

johnrom commented Dec 1, 2020

@maddhruv that is one of the many different package configurations I listed, all of which will have different effects.

if your project used lodash-es (which we have previously recommended people use to fix this very issue), you would receive newly duplicated packages with this PR, for example

I still agree with this change, though as mentioned above I think we should go above and beyond to limit the scope of our imports since lodash isn't tree-shakeable (I don't think? maybe changed in future webpack versions) and make sure that webpack lodash plugin can optimize them to create the lowest total package size across all common project types. This will also affect people's packages in a big way so this is likely something we should punt to v3.

@jaredpalmer
Copy link
Owner

TSDX intelligently bundles lodash and treeshakes properly.

https://tsdx.io/optimization#using-lodash

However, I would like to get rid of lodash altogether. I haven't yet found an equivalent of toPath that supports brackets.

@TrySound
Copy link
Collaborator

TrySound commented Dec 2, 2020

@jaredpalmer I mean will tsdx continue to work when lodash-es is removed?

@jaredpalmer
Copy link
Owner

Haven't tried, my guess is it will not.

@jaredpalmer
Copy link
Owner

AFAIK implementation is still your original one from back when formik handled its own build.

@palfrey
Copy link

palfrey commented Dec 8, 2020

lodash-es hasn't had a release since 4.17.15 and so is subject to a bunch of vulnerabilities v.s. lodash 4.17.20 which has none of them. See https://snyk.io/test/npm/lodash/4.17.15

@TrySound
Copy link
Collaborator

TrySound commented Dec 8, 2020

Someday they may lost an access to lodash package as well 🤷‍♂️

@johnrom
Copy link
Collaborator

johnrom commented Dec 8, 2020

I don't think anyone is advocating that we keep lodash-es for no reason, but I do recommend a bunch of mitigations above that need to be considered before merging this, and this should probably be shipped with v3 and not v2 for those who have already solved the duplication in their projects another way, unless we can prove in all situations I listed above this doesn't have a negative impact.

@jaredpalmer
Copy link
Owner

I am open to removing lodash in v3 but we need to do it gracefully with warnings in v2 if there are any backwards incompatible changes (such as bracket path references)

@github-actions
Copy link
Contributor

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Jan 17, 2021
@palfrey
Copy link

palfrey commented Jan 17, 2021

lodash-es hasn't had a release since 4.17.15 and so is subject to a bunch of vulnerabilities v.s. lodash 4.17.20 which has none of them. See https://snyk.io/test/npm/lodash/4.17.15

This bit looks fixed now. See https://github.com/lodash/lodash/issues/4879#issuecomment-748571123

@github-actions github-actions bot removed the stale label Jan 18, 2021
@l0gicgate
Copy link

Maybe better completely get rid from lodash?

@TrySound I'm 💯 in favor of this. 75kb (ungzipped) is a lot..

lodash-es

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2021

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Mar 7, 2021
@johnrom johnrom removed the stale label Mar 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2021

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Apr 8, 2021
@johnrom johnrom removed the stale label Apr 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2021

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label May 9, 2021
@github-actions github-actions bot closed this Jul 8, 2021
@johnrom johnrom reopened this Jul 8, 2021
@johnrom johnrom removed the stale label Jul 8, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2021

This pull request is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 60 days

@github-actions github-actions bot added the stale label Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants