-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
feat(webpack-cli): added mode argument #1253
Conversation
97ae08e
to
659a243
Compare
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.
Some suggestions, PTAL
test/mode/prod/prod.test.js
Outdated
const { stat } = require('fs'); | ||
const { resolve } = require('path'); | ||
describe('mode flags', () => { | ||
it('should load a development config when --prod is passed', done => { |
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.
it('should load a development config when --prod is passed', done => { | |
it('should load a production config when --prod is passed', done => { |
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.
Similarly in all tests below
test/mode/prod/prod.test.js
Outdated
}); | ||
|
||
it('should load a development config when --mode=production is passed', done => { | ||
const { stderr, stdout } = run(__dirname, ['--prod', 'production']); |
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.
Did you mean?
const { stderr, stdout } = run(__dirname, ['--prod', 'production']); | |
const { stderr, stdout } = run(__dirname, ['--mode', 'production']); |
expect(stats.isFile()).toBe(true); | ||
done(); | ||
}); | ||
}); |
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.
You can add another case of using both --prod and --dev which should use prod config.
659a243
to
252f50e
Compare
@ematipico Thanks for your update. I labeled the Pull Request so reviewers will review it again. @anshumanv Please review the new changes. |
@anshumanv thank you for your inputs! PR updated :) |
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 good!
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 good to me!
What kind of change does this PR introduce?
Feature, it adds back
mode
argumentDid you add tests for your changes?
Yes
If relevant, did you update the documentation?
Yes
Summary
People asked to have back the
mode
argument.Does this PR introduce a breaking change?
No
Other information