-
-
Notifications
You must be signed in to change notification settings - Fork 602
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(cli): throws err when no args passed #862
Conversation
trows an error when webpack is not installed and no args passed to the cli
fixed spacing issues
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.
Left a few suggestions! Thanks.
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 think there are two user cases:
- webpack not installed and no args passed
- webpack intalled and no args passed
tells user to install webpack in order for futher procedures
shows different error message based on package manager
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.
Looks like it works from the discussion, but code can be shaped better if you want to improve it
combined nested conditional blocks into one
Yes thats what i meant, thank you for implementing 👍 |
fixed linting error
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
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.
Looks great now, left a suggestion :)
Need help: Reason for failing CI |
Please rebase, we removed node 6 from the CI |
@pranshuchittora Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
modified the default stack trace limit and made cli to exit with exit code 1 when no args passed and webpack is not installed
removes the return statement from the catch block
removed the comment as per the suggestions by even
bin/cli.js
Outdated
@@ -70,6 +70,20 @@ For more information, see https://webpack.js.org/api/cli/.`); | |||
try { | |||
options = require("./utils/convert-argv")(argv); | |||
} catch (err) { | |||
if (err.code === "MODULE_NOT_FOUND") { | |||
let errorMessage = | |||
"\n\u001b[31mwebpack not found, \u001b[33mplease install webpack using\n\t\u001b[32mnpm install --save-dev webpack\n"; |
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.
webpack not installed, consider installing it using npm install --save-dev webpack
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.
Indentation seems weird here, you don't need different color schemes for the not found
part, the please install
part and the installation text. Two colors is sufficient, one for the error stack and one for the npm save
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.
Done
updates the err message
@evenstensberg pls review the final changes. |
fixes the linting err
Tests are failing, could you revise? Having a look now at how it's looking |
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.
lgtm, do you mind fixing the eslint stuff? Other than that I think we're ready to merge
console.error(errorMessage); | ||
Error.stackTraceLimit = 1; | ||
process.exitCode = 1; | ||
return; |
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 guess this is fine, as long as we're doing it in the scope of only MODULE_NOT_FOUND
Closes #861
** Behaviour after fix **
What kind of change does this PR introduce?
bugfix
Did you add tests for your changes?
Nopes