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

Jump to bottom

refactor: remove lodash, add fast-deep-equal #6496

Merged
merged 4 commits into from Jun 26, 2020
Merged

refactor: remove lodash, add fast-deep-equal #6496

merged 4 commits into from Jun 26, 2020

Conversation

JamieMagee
Copy link
Contributor

With the help of You don't need lodash. Exceptions are:

  • Replaced _.isEqual with fast-deep-equal
  • Reimplemented sampleSize
  • Reimplemented camelCase

Done as part of my investigation into jest memory leakage. Unfortunately it's brought in via a lot of our dependencies, but it helps a bit.

viceice
viceice requested changes Jun 12, 2020
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

needs lint fixes too

lib/versioning/ruby/version.ts Outdated Show resolved Hide resolved
lib/versioning/ruby/version.ts Outdated Show resolved Hide resolved
@rarkins rarkins changed the title refactor: remove lodash refactor: remove lodash and add fast-deep-equal lol Jun 12, 2020
@JamieMagee
Copy link
Contributor Author

needs lint fixes too

Yeah, I realised just after I sent it out that I'd been testing with just yarn jest

rarkins
rarkins reviewed Jun 12, 2020
View reviewed changes
lib/workers/pr/index.ts Outdated Show resolved Hide resolved
rarkins
rarkins reviewed Jun 12, 2020
View reviewed changes
yarn.lock Outdated
@@ -6376,7 +6381,7 @@ lodash.without@~4.4.0:
resolved "https://registry.yarnpkg.com/lodash.without/-/lodash.without-4.4.0.tgz#3cd4574a00b67bae373a94b748772640507b7aac"
integrity sha1-PNRXSgC2e643OpS3SHcmQFB7eqw=

lodash@4.17.15, lodash@^4.17.11, lodash@^4.17.13, lodash@^4.17.14, lodash@^4.17.15, lodash@^4.17.4:
lodash@^4.17.11, lodash@^4.17.13, lodash@^4.17.14, lodash@^4.17.15, lodash@^4.17.4:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we still using lodash in production indirectly?

Copy link
Member

Choose a reason for hiding this comment

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

image

yes, indirect by yawn-yaml

Copy link
Collaborator

Choose a reason for hiding this comment

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

image

@JamieMagee JamieMagee force-pushed the refactor/remove-lodash branch 2 times, most recently from e0a388b to 1382f59 Compare June 12, 2020 18:20
@JamieMagee
Copy link
Contributor Author

@rarkins Did something change in logger? I didn't change anything there, but all the lint errors are around traverse in logger.

@rarkins
Copy link
Collaborator

rarkins commented Jun 17, 2020

@JamieMagee not that I can recall. Not according to GitHub either: https://github.com/renovatebot/renovate/commits/master/lib/logger

@JamieMagee JamieMagee force-pushed the refactor/remove-lodash branch 2 times, most recently from 64af4ca to 4a8105b Compare June 18, 2020 20:54
@JamieMagee
Copy link
Contributor Author

It's because I created a dependency from tools to lib. Tools is compiled with the default tsconfig.json (really just tsc -c tools) so it type checks a lot of lib and fails as a result.

@JamieMagee
Copy link
Contributor Author

Needs coverage fixes.

@viceice
Copy link
Member

viceice commented Jun 19, 2020

It's because I created a dependency from tools to lib. Tools is compiled with the default tsconfig.json (really just tsc -c tools) so it type checks a lot of lib and fails as a result.

?? There should be no dep from tools to lib. Tools folder has its own tsconfig which should be used to compile only files in tools.

@JamieMagee
Copy link
Contributor Author

?? There should be no dep from tools to lib.

True, but I accidentally added one when refactoring this code and moving the camelCase function to lib/util/strings. I've reverted this now.

@rarkins rarkins changed the title refactor: remove lodash and add fast-deep-equal lol refactor: remove lodash, add fast-deep-equal Jun 26, 2020
@JamieMagee
refactor: remove lodash
b5061ec 
With the help of [You don't need lodash](https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore). Exceptions are:

- Replaced _.isEqual with [fast-deep-equal](https://github.com/epoberezkin/fast-deep-equal)
- Reimplemented sampleSize
- Reimplemented camelCase

Done as part of my investigation into jest memory leakage. Unfortunately it's brought in via a lot of our dependencies, but it helps a bit
@JamieMagee
test coverage
740796e
viceice
viceice reviewed Jun 26, 2020
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Just one question, otherwise LGTM

lib/datasource/index.ts Show resolved Hide resolved
@JamieMagee JamieMagee merged commit 350a70f into master Jun 26, 2020
@JamieMagee JamieMagee deleted the refactor/remove-lodash branch June 26, 2020 09:31
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 21.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@viceice viceice viceice approved these changes

@rarkins rarkins Awaiting requested review from rarkins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JamieMagee @rarkins @viceice @renovate-release