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.

Sign up for GitHub

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

Jump to bottom

fix: Upgrade yargs to fix security issue and more #18

Merged
merged 1 commit into from
Dec 22, 2020
Merged

fix: Upgrade yargs to fix security issue and more #18

merged 1 commit into from
Dec 22, 2020

Conversation

danez
Copy link
Contributor

@danez danez commented Dec 15, 2020 •
edited

  • update yargs because of https://www.npmjs.com/advisories/1500
  • Use yargs itself to validate args instead of custom code. Gives nice --help and nice errors.
  • replace glob with fast-glob which has promise support and should be faster
  • update csvtojson to version 2 which now also has promise support and is 8-10x faster (according to changelog)
  • change error handling slightly
    • error if no reports were found.
    • global catch clause to catch all errors that might happen
    • above removes the need to have try-catch

Fixes #10 ?

@danez
fix: Upgrade yargs to fix security issue
ccbefee 
Use yargs to validate args
replace glob with fast-glob
update csvtojson to version 2
danez
danez commented Dec 15, 2020
View reviewed changes
index.js
@@ -110,23 +96,20 @@ const convertFileRows = (rows) => rows
const convertAllFileRows = (allFileRows) => allFileRows.map(convertFileRows);

const concatAllFileRows = (allFileRows) => allFileRows
.reduce((combined, fileRows) => combined.concat(fileRows));
.reduce((combined, fileRows) => combined.concat(fileRows), []);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did throw if there are no rows found, hence the initial value.

@galvarez421 galvarez421 self-requested a review December 17, 2020 01:08
@galvarez421 galvarez421 self-assigned this Dec 17, 2020
@galvarez421 galvarez421 added the enhancement New feature or request label Dec 17, 2020
@galvarez421
Copy link
Member

Thank you for the contribution! It looks good from reading through it. I will review and test and hopefully merge and release updated package by this weekend.

@galvarez421 galvarez421 changed the base branch from master to develop December 22, 2020 15:40
@galvarez421 galvarez421 merged commit e2ffb53 into browserslist:develop Dec 22, 2020
@danez danez deleted the update branch December 22, 2020 16:14
@galvarez421 galvarez421 mentioned this pull request Dec 22, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@galvarez421 galvarez421 Awaiting requested review from galvarez421

Assignees

@galvarez421 galvarez421

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor error handling
2 participants
@danez @galvarez421