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 dep #3198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

remove lodash dep #3198

wants to merge 1 commit into from

Conversation

jimmywarting
Copy link
Contributor

I don't think using just one lodash fn don't justify depending on it.

@XhmikosR
Copy link
Contributor

Hmm, perhaps it's not needed in https://github.com/dlmanning/gulp-sass either. I switched from lodash to lodash.clonedeep, but maybe it's not needed there either.

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Dec 27, 2021

@XhmikosR

use of lodash.clonedeep and similar packages is discouraged and they will be removed in v5.

ref: https://lodash.com/per-method-packages

maybe a simple JSON.parse(JSON.stringify(...)) is sufficient?
V8 aggressively optimized this pattern

ref: https://web.dev/structured-clone

@XhmikosR
Copy link
Contributor

Thanks for the reply.

I'm aware that the individual packages are sort of deprecated, but in this case I don't think it matters.

Either way, if you are up to it, happy to see one less dependency in gulp-sass too. It's already improved with the lodash -> lodash.clonedeep switch on main branch.

@xzyfer
Copy link
Contributor

xzyfer commented Dec 28, 2021

Thanks but Object.assign is not equivalent to clonedeep and I'm not confident we have adequate test coverage to assert this is a safe behaviour change.

@XhmikosR
Copy link
Contributor

XhmikosR commented Feb 2, 2022

This needs to be rebased @jimmywarting but any chance you land this after this rebase @xzyfer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants