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

Address security warnings, upgrade dependencies, use npm-run-all. #283

Merged
merged 3 commits into from
Apr 24, 2019

Conversation

parkerziegler
Copy link
Contributor

@parkerziegler parkerziegler commented Apr 23, 2019

This PR was inspired a bit by #282, which incentivized me to see if we had any other security vulnerabilities. It turns out we did 😬Running yarn audit on master currently outputs the following:

image

After taking a look at the security warnings on npm, it became clear we could avoid these just by bumping a few of our dependencies. Running yarn audit on this branch yields no security warnings.

I also decided to make two other changes in this PR:

  1. Use text as our command line coverage reporter for nyc. This gives us nice reporting on lines missing coverage and matches the Jest UI (which I find helpful when strategizing about what to test).

image

  1. Include npm-run-all for our check and check-ci scripts. I like npm-run-all because it will detect what client you're using (npm or yarn) and use it for you to execute the scripts. I considered enforcing yarn here but realized this is often a polarizing enough decision that it might turn off contributors. For example, contributors can still npm install the project if they really want to, but if we used yarn in all of these package.json scripts, they flat out wouldn't work for npm users. npm-run-all is a nice in-between that keeps everyone happy. It exposes run-s which will run the commands in sequence (it also exposes run-p which will do things in parallel).

Copy link
Member

@ryan-roemer ryan-roemer left a comment

Choose a reason for hiding this comment

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

🚀 🛡

@parkerziegler parkerziegler merged commit 981fd84 into master Apr 24, 2019
@parkerziegler parkerziegler deleted the task/address-security-warnings branch April 24, 2019 18:47
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.

2 participants