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

Use minimist instead of yargs to get rid of dependencies #23

Merged
merged 11 commits into from
Apr 16, 2023

Conversation

CW-B-W
Copy link
Contributor

@CW-B-W CW-B-W commented Apr 15, 2023

Hello @9bany ,

Thanks for your work, I am planning to use curl-to-json to add curl support for Motrix, but the dependency of yargs causes the following warnings in Electron.

WARNING in ./node_modules/yargs-parser/build/index.cjs 1029:19-32
Critical dependency: the request of a dependency is an expression
WARNING in ./node_modules/yargs/build/index.cjs 1:437-463
Critical dependency: the request of a dependency is an expression
.
.
.

Therefore I adapted it to minimist to get rid of the warnings and further reduces the large number of dependencies.

I have tested it with my Motrix branch and it works fine, but I don't know whether it affects any feature of your project.

Hope it helps!

src/pare-json.js Outdated Show resolved Hide resolved
Copy link
Owner

@9bany 9bany left a comment

Choose a reason for hiding this comment

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

Thank for your work @CW-B-W
Cool pull request, i want you add unit test before i merge it.

Thank you again for your work, everything's cool.

@CW-B-W
Copy link
Contributor Author

CW-B-W commented Apr 16, 2023

Thank for your work @CW-B-W Cool pull request, i want you add unit test before i merge it.

Thank you again for your work, everything's cool.

Thanks for you suggestions, I found bugs in parsing the --data option, will fix and test it.

@CW-B-W
Copy link
Contributor Author

CW-B-W commented Apr 16, 2023

Hello @9bany,

I have fixed the bugs & add test/matcher.matchArgv.test.js for the regex matching unit test.

The output of test/parse.test.js of this version is identical to that of yargs version.

Please review it again, thanks!

Copy link
Owner

@9bany 9bany left a comment

Choose a reason for hiding this comment

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

LGTM!

@9bany 9bany merged commit 476529f into 9bany:master Apr 16, 2023
@9bany
Copy link
Owner

9bany commented Apr 16, 2023

Just release on latest version, thanks @CW-B-W

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