Navigation Menu

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(dep): migrate from lodash.set to lodash. #2306

Merged
merged 4 commits into from Jun 3, 2022
Merged

Conversation

mastermatt
Copy link
Member

Listing lodash as a prod dependency has a few advantages over the single func dep we were using.
Primarily, when a security vulnerability is patched in LoDash, those changes don't get published to individual function libs anymore.
The most recent example being the prototype pollution that was patched in 4.17.16.
That particular vulnerability didn't effect Nock because of our usage, however, Nock does show up as having a vulnerability because of it which is a pain for consumers. It also creates a future problem where the next patch could affect Nock more directly.
#2279

It should be noted that LoDash itself discourages the use of these single function libs: https://lodash.com/per-method-packages

This changes also removes the vendor-izing of two LoDash functions in favor of using the lib directly (isPlainObject and mapValues).

Listing `lodash` as a prod dependency has a few advantages over the single func dep we were using.
Primarily, when a security vulnerability is patched in LoDash, those changes don't get published to individual function libs anymore.
The most recent example being the prototype pollution that was patched in 4.17.16.
That particular vulnerability didn't effect Nock because of our usage, however, Nock does show up as having a vulnerability because of it which is a pain for consumers. It also creates a future problem where the next patch could affect Nock more directly.
#2279

It should be noted that LoDash itself discourages the use of these single function libs: https://lodash.com/per-method-packages

This changes also removes the vendor-izing of two LoDash functions in favor of using the lib directly (`isPlainObject` and `mapValues`).
@mastermatt
Copy link
Member Author

Normally, dependency updates would be a chore, but I committed this with a fix prefix so a new patch version would get published if this is merged.

@mastermatt
Copy link
Member Author

@paulmelnikow I know you were in favor of lodash.set when we first added it. Any rebuttal to this PR?

@mastermatt mastermatt added the dependencies Pull requests that update a dependency file label Feb 17, 2022
@mastermatt mastermatt requested a review from a team February 17, 2022 18:20
Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

maybe there is an alternative single-purpose modules that we can use instead of lodash.set, instead of adding all of lodash?

When in doubt, I'm okay with just adding lodash, we can look into this later

@zsepton
Copy link

zsepton commented Mar 31, 2022

Hey @mastermatt I was wondering if this fix can be reviewed & merged, lodash won't be supporting individual packages anymore. It will be great if nock will be updated to remove this snyk vulnerability.
Thanks

@mastermatt mastermatt requested a review from a team March 31, 2022 17:13
@mastermatt
Copy link
Member Author

I'm in favor of the current state of this PR, however, I need another maintainer to approve before it can be merged.

With this change, the full LoDash dep is used by Node apps, which tend to have LoDash anyway and where package size isn't much of a consideration, and can use proper tree-shaking by anyone what cares abound bundle size.

@candrews
Copy link

bump

I'd love to see this MR merged - can this please be progressed? Thank you all in advance!

@zsepton
Copy link

zsepton commented Jun 2, 2022

bump

can this please be progressed? Thank you all in advance!

@mastermatt
Copy link
Member Author

@nock/maintainers anyone else want to 👍🏼

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Good with me, but all tests seem to be failing? Errors seem unrelated to current PR though, so feel free to merge & release. I'd appreciate if someone could help find out what's up with the tests though, I'm very limited due to family and work right now

@mastermatt
Copy link
Member Author

@gr2m check my comment on this pr about the tests #2364 (comment)
I guess we don't need to wait on that fix for this.

@mastermatt mastermatt merged commit e4b0331 into main Jun 3, 2022
@mastermatt mastermatt deleted the update-lodash branch June 3, 2022 17:49
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

🎉 This PR is included in version 13.2.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants