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

fix: resolve opts when no-config & fix vulns #1024

Merged
merged 5 commits into from Aug 18, 2019
Merged

Conversation

evenstensberg
Copy link
Member

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

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
bin/utils/convert-argv.js Show resolved Hide resolved
}

// process Promise
if (typeof options.then === "function") {
if (options && typeof options.then === "function") {
Copy link
Contributor

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

Copy link
Member Author

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)

Copy link
Contributor

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.

Copy link
Member Author

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>
@webpack-bot
Copy link

@evenstensberg Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

evenstensberg and others added 2 commits August 14, 2019 17:15
Co-Authored-By: Emanuele <my.burning@gmail.com>
Copy link
Contributor

@ematipico ematipico left a 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

Copy link

@ryanspice ryanspice left a 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

@evenstensberg evenstensberg merged commit b20ecd3 into master Aug 18, 2019
@evenstensberg evenstensberg deleted the fix/no-config-mode branch August 18, 2019 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants