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

Remove lodash? #2279

Closed
danielnixon opened this issue Jul 7, 2020 · 11 comments · Fixed by #3478
Closed

Remove lodash? #2279

danielnixon opened this issue Jul 7, 2020 · 11 comments · Fixed by #3478
Labels
dependencies Issue about dependencies of the package help wanted Extra attention is needed package: typescript-estree Issues related to @typescript-eslint/typescript-estree

Comments

@danielnixon
Copy link
Contributor

Lodash has an open security vuln and shows signs of being borderline unmaintained.

Repro

  1. Install typescript-eslint/eslint-plugin
  2. Check your Snyk report (e.g. https://snyk.io/test/github/danielnixon/eslint-plugin-total-functions?targetFile=package.json)
  3. Or run yarn audit / npm audit

Expected Result

No security vuln reported

Actual Result

Lodash security vuln reported

Additional Info

It looks like typescript-estree only uses lodash once, for unescape. unescape happens to be tiny and unlikely to evolve over time: https://github.com/lodash/lodash/blob/4.17.11/lodash.js#L15145

I'd be happy to raise a PR to inline unescape (or maybe replace it with https://www.npmjs.com/package/he or something) and remove the lodash dependency.

Versions

Latest

@danielnixon danielnixon added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for maintainers to take a look labels Jul 7, 2020
@bradzacher
Copy link
Member

bradzacher commented Jul 8, 2020

Sure - happy to inline it if you want.
TBH I'm not even sure if this is required - I don't know what this check even does:

} else {
result.value = unescapeStringLiteralText(node.text);
}

Maybe we don't even need to inline it and can just delete it altogether? Unsure.
Worth having a play around with - happy to accept a PR!

@bradzacher bradzacher added dependencies Issue about dependencies of the package and removed triage Waiting for maintainers to take a look labels Jul 8, 2020
@dman777
Copy link

dman777 commented Jul 22, 2020

I second that request to remove lodash bloat

@bradzacher bradzacher added the help wanted Extra attention is needed label Jul 22, 2020
@papb
Copy link
Contributor

papb commented Jul 23, 2020

I think 'borderline unmaintained' is very exaggerated. He is just looking for new maintainers to help.

@danielnixon
Copy link
Contributor Author

I think 'borderline unmaintained' is very exaggerated.

Perhaps, but

seems to me like an intolerable timeline whatever you call it.

See also https://github.com/lodash/lodash/issues/4738#issuecomment-629698149 which I tend to agree with.

@JoshuaKGoldberg
Copy link
Member

@danielnixon still planning on sending a PR for this? I can if not!

@danielnixon
Copy link
Contributor Author

Please go for it @JoshuaKGoldberg

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Sep 8, 2020

Fun fact: typescript-eslint actually uses two lodash members: memoize and unescape. I thought about this more, and a few thoughts against removing lodash come to mind:

  • IMO there's still benefit to the "lodash ideology" of using the same stable functions & high quality docs across projects. Why bother recreating those methods internally when they're already available & well-documented?
  • Lodash is already a transitive dependency many times over in yarn.lock, so removing it from direct usage does very little.

Consider this to be me "unassigning" myself from this issue, as much as a non-maintainer random contributor can. 😉

@danielnixon
Copy link
Contributor Author

memoize is in eslint-plugin-tslint which will hopefully die off one day so I didn't consider it a priority for this ticket (just typescript-estree here).

Your other points are good. 👍 🤔

@dman777
Copy link

dman777 commented Sep 8, 2020

IMO there's still benefit to the "lodash ideology" of using the same stable functions & high quality docs across projects. Why bother recreating those methods internally when they're already available & well-documented?

The same applies to native JS and more so because JS is on everything and anything JS while lodash is not.

@bradzacher
Copy link
Member

In the next release, typescript-estree will no longer have a dependency on lodash, thanks to @armano2.

@bradzacher
Copy link
Member

There is still memoize in eslint-plugin-tslint

And in the interim one usage has been introduced into eslint-plugin (iirc this was due to a util from eslint that was inlined)

import escapeRegExp from 'lodash/escapeRegExp';

@armano2 armano2 added help wanted Extra attention is needed and removed has pr there is a PR raised to close this labels Feb 7, 2021
@armano2 armano2 removed their assignment Feb 8, 2021
@bradzacher bradzacher linked a pull request Jun 3, 2021 that will close this issue
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dependencies Issue about dependencies of the package help wanted Extra attention is needed package: typescript-estree Issues related to @typescript-eslint/typescript-estree
Projects
None yet
6 participants