-
-
Notifications
You must be signed in to change notification settings - Fork 621
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
fix: resolve opts when no-config & fix vulns #1024
Conversation
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
bin/utils/convert-argv.js
Outdated
} | ||
|
||
// process Promise | ||
if (typeof options.then === "function") { | ||
if (options && typeof options.then === "function") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you have the if/else, options
will never be undefined right? So this additional check is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checks for options.then
object prop so will throw. We're setting props on the object later these checks, I think we "assumed" we either found or did not find the props before validating args that aren't protected by 0CJS (entry and output)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not talking about .then, I'm talking about options &&
. It's redundant. You already assigned it before with options = {}
.
So or you keep this check and remove the assignment or keep the assignment and remove the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing it out, changed now!
Co-Authored-By: Emanuele <my.burning@gmail.com>
@evenstensberg Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
Co-Authored-By: Emanuele <my.burning@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review assignment/check of options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pulled, tested, and compiles for me now after experiencing #1023
What kind of change does this PR introduce?
Closes #1023
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
N/A
Summary
Dupes out of resolving options if there are none to throw an error
Does this PR introduce a breaking change?
No
Other information
N/A