-
-
Notifications
You must be signed in to change notification settings - Fork 622
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: check webpack installation before running cli #1827
Conversation
let cli = undefined; | ||
if (packageExists('webpack')) { | ||
cli = require('webpack').cli; | ||
} |
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.
Why here?
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.
Can be refactored as -> const cli = packageExists('webpack') ? require('webpack').cli : undefined
Else we get Error: can't find module 'webpack'
@@ -4,13 +4,21 @@ | |||
require('v8-compile-cache'); | |||
const importLocal = require('import-local'); | |||
const runCLI = require('../lib/bootstrap'); | |||
const { yellow } = require('colorette'); | |||
const { packageExists, promptInstallation } = require('@webpack-cli/package-utils'); |
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.
Do we add package-utils
in deps?
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.
Yes Already present.
"@webpack-cli/package-utils": "^1.0.1-rc.0", |
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.
But these utils are only being used by webpack-cli
, So we can safely move it from package-utils
to utils
. Let's do this in separate PR.
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.
#1822 will solve this
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.
Good idea!
8471606
to
36e75ab
Compare
36e75ab
to
eecd833
Compare
@snitin315 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @rishabh3112 Please review the new changes. |
What kind of change does this PR introduce?
fix
Did you add tests for your changes?
no
If relevant, did you update the documentation?
no
Summary
prompt to install
webpack
if it's not present already.Does this PR introduce a breaking change?
No
Other information
Refers #1383 (comment)
Fixes #1383