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

Replace lodash with the lodash.clonedeep package #818

Merged
merged 1 commit into from Oct 6, 2021
Merged

Replace lodash with the lodash.clonedeep package #818

merged 1 commit into from Oct 6, 2021

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Oct 4, 2021

Notes:

  1. lodash isn't in any of the current dependencies' deps
  2. I'm unsure if the individual packages are still maintained, though. Alternatively, we could find another package.

https://packagephobia.com/result?p=lodash%2Clodash.clonedeep

@XhmikosR XhmikosR marked this pull request as draft October 4, 2021 13:13
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 6, 2021

I pushed another patch which switches to the extend package. I don't have a preference, both should do the job and are small.

https://packagephobia.com/result?p=lodash%2Clodash.clonedeep%2Cextend

index.js Outdated
@@ -30,7 +30,7 @@ const gulpSass = (options, sync) => transfob((file, enc, cb) => { // eslint-disa
return cb(null, file);
}

const opts = clonedeep(options || {});
const opts = extend(true, options || {});
Copy link
Contributor Author

@XhmikosR XhmikosR Oct 6, 2021

Choose a reason for hiding this comment

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

I think we can also drop the fallback || {} since extend handles this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 6, 2021

The CI failure is unrelated to these changes; GH Actions return the paths as D or d for some reason inconsistently sometimes.

@XhmikosR XhmikosR changed the title Switch to the lodash.clonedeep package Remove lodash Oct 6, 2021
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 6, 2021

@xzyfer can you let me know how I should proceed please?

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 6, 2021

But with which solution 😛?

@xzyfer
Copy link
Collaborator

xzyfer commented Oct 6, 2021

I'm happy with either. It's fairly inconsequential as long as tests pass.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 6, 2021

The thing is that tests don't catch everything :)

Anyway, I went with extend because it's also a dependency of other packages one will probably have:

C:\Users\xmr\Desktop\gulp-sass>npm ls extend
gulp-sass@5.0.0 C:\Users\xmr\Desktop\gulp-sass
+-- extend@3.0.2
+-- gulp@4.0.2
| +-- gulp-cli@2.3.0
| | `-- liftoff@3.1.0
| |   `-- extend@3.0.2  deduped
| `-- vinyl-fs@3.0.3
|   `-- glob-stream@6.1.0
|     `-- extend@3.0.2  deduped
`-- node-sass@6.0.1
  `-- request@2.88.2
    `-- extend@3.0.2  deduped

lodash.clonedeep:

C:\Users\xmr\Desktop\gulp-sass>npm ls lodash.clonedeep
gulp-sass@5.0.0 C:\Users\xmr\Desktop\gulp-sass
`-- eslint@7.32.0
  `-- table@6.7.2
    `-- lodash.clonedeep@4.5.0

@xzyfer
Copy link
Collaborator

xzyfer commented Oct 6, 2021 via email

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Oct 6, 2021

NVM, I think we better play it safe and go with lodash.clonedeep. We can revisit this later.

@XhmikosR XhmikosR marked this pull request as ready for review October 6, 2021 06:46
@XhmikosR XhmikosR changed the title Remove lodash Replace lodash with the lodash.clonedeep package Oct 6, 2021
@xzyfer xzyfer merged commit 0b6e102 into dlmanning:master Oct 6, 2021
@XhmikosR XhmikosR deleted the lodash branch October 6, 2021 07:00
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

2 participants