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

Upgrade and reduce the usage of lodash #141

Closed

Conversation

alexbjorlig
Copy link
Contributor

@alexbjorlig alexbjorlig commented Feb 10, 2021

Objective --> remove lodash security warnings

The primary objective of this PR is to remove security warnings when using the library (maybe we can setup snyk.io later to warn about this?)

What changed

Simply upgrading lodash was not easy at all, and gave a tremendous amount of errors because a lot of functions were re-named or their API was modified.

In response to this, I converted must lodash functions to just using Node.js.

However there are 2 functions left:

  • _.get()
  • _.defaultsDeep()

I think get can be easily removed, when we upgrade the repo to Node.js v14, because then we can use ?. and ?? syntax. I'm not so sure about defaultsDeep.

But I decided to finish up the PR because, hey it works and Lodash is updated to the latest version 💃🏾 🚀

Positive side-effects

I actually think the source code will be more easy to read with the reduced usage of lodash.

Closes #135

Only function I could not re-write was defaults-deep.
I moved the function to a utils folder, so we can remove it in the
near future
I could not completly remove lodash, need help with 2 function calls:

    - stream.js
        --> _.get
        --> _.defaultsDeep
@alexbjorlig alexbjorlig force-pushed the upgrade-and-reduce-use-lodash branch from df6bb6a to a87316f Compare February 11, 2021 07:50
Maybe this can solve the CI issue?
@alexbjorlig
Copy link
Contributor Author

This is a rather big PR - almost done (still working on some small issues with Travis).

Because of the refactor to modern js --> the old way of using istanbul and mocha was failing in CI. I therefore also updated their API according to newest documentation (the istanbul CLI might not have been around back in the days ☺️).

I hope it will be passing soon.

@alexbjorlig
Copy link
Contributor Author

@addaleax can you maybe take a look at why the CI process fails?

@addaleax
Copy link
Contributor

I don’t know, it seems like just re-pushing might help? (I can’t restart the Travis workflow myself.)

Also, if you want, you can PR the Github Workflow separately. I’m not sure if Github needs to have one present on the master branch in order to work, but if that works, we can just remove Travis entirely (which we’ve been doing for other repositories as well).

@alexbjorlig
Copy link
Contributor Author

I don’t know, it seems like just re-pushing might help? (I can’t restart the Travis workflow myself.)

Also, if you want, you can PR the Github Workflow separately. I’m not sure if Github needs to have one present on the master branch in order to work, but if that works, we can just remove Travis entirely (which we’ve been doing for other repositories as well).

Sounds like a good plan - here is the pr with only the updated ci / github workflow #144

@alexbjorlig
Copy link
Contributor Author

I don’t know, it seems like just re-pushing might help? (I can’t restart the Travis workflow myself.)

Also, if you want, you can PR the Github Workflow separately. I’m not sure if Github needs to have one present on the master branch in order to work, but if that works, we can just remove Travis entirely (which we’ve been doing for other repositories as well).

PR incoming wo😅

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.

Security issues
2 participants