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.

Sign up for GitHub

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

Jump to bottom

[Refactor] use native JS instead of lodash #1419

Merged
merged 5 commits into from Jul 17, 2019
Merged

[Refactor] use native JS instead of lodash #1419

merged 5 commits into from Jul 17, 2019

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Jul 17, 2019 •
edited

This removes a large dep that often has CVEs, in exchange for using native JS which is clearer and more maintainable.

Closes #1224.

@ljharb
[Refactor] importType: remove use of cond
c5078ad 
Using normal JS makes for far more understandable and maintainable code.
@ljharb
[Refactor] no-extraneous-dependencies: use Array.isArray instead …
8a38fd4 
…of lodash
@ljharb
[Refactor] no-extraneous-dependencies: remove the last bit of lodash
d3a3fa5 
Using `.length` is clearer than a polymorphic `isEmpty`.
@coveralls
Copy link

coveralls commented Jul 17, 2019 •
edited

Coverage Status

Coverage increased (+0.007%) to 97.96% when pulling 5abd5ed on ljharb:remove_lodash into 118afd4 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 95.224% when pulling 915f571 on ljharb:remove_lodash into 6512110 on benmosher:master.

@ljharb
Copy link
Member Author

ljharb commented Jul 17, 2019 •
edited

Going to go ahead and merge once tests all pass; the changes are minimal.

The last commit also hopefully fixes the failing tests on master.

@ljharb
Copy link
Member Author

ljharb commented Jul 17, 2019

Some eslint 3 tests still failing; I'll fix those and get tests passing on this PR prior to merging.

benmosher
benmosher approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@benmosher benmosher left a comment

Choose a reason for hiding this comment

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

👍🏼 though heads up, I also just pushed a fix to master to fix the no-deprecated tests, basically the same fix you have here

@benmosher
Copy link
Member

The test failures are pretty weird. No idea there.

FWIW the webpack resolver still has a lodash reference, but it's declared in its package.json so I would expect that to be unrelated?

@golopot
Copy link
Contributor

golopot commented Jul 17, 2019

https://github.com/benmosher/eslint-plugin-import/blob/6a07c8142c79044a83a1949a17d41a2390833b59/tests/src/rules/no-deprecated.js#L212

typescript-eslint-parser@22.0.0 requires eslint >= 4, maybe that's it?

https://unpkg.com/typescript-eslint-parser@22.0.0/package.json

@golopot
Copy link
Contributor

golopot commented Jul 17, 2019

This pr closes #1224 ?

@ljharb
Copy link
Member Author

ljharb commented Jul 17, 2019

I can repro these failures locally - but when I check out latest master, they still fail :-/

@ljharb ljharb merged commit 5abd5ed into import-js:master Jul 17, 2019
@ljharb ljharb deleted the remove_lodash branch July 17, 2019 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lencioni lencioni lencioni approved these changes

@benmosher benmosher benmosher approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ljharb @coveralls @benmosher @golopot @lencioni