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.

Sign up for GitHub

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

Jump to bottom

feat(webpack-cli): add no-mode flag #1276

Merged
merged 2 commits into from
Mar 3, 2020
Merged

feat(webpack-cli): add no-mode flag #1276

merged 2 commits into from
Mar 3, 2020

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Feb 27, 2020 •
edited

What kind of change does this PR introduce?

feature - add no-mode flag

Did you add tests for your changes?
Work in progress.

If relevant, did you update the documentation?
Yes

Summary

Refers #1240

Refers #1012

Does this PR introduce a breaking change?

Other information

@snitin315 snitin315 requested a review from a team as a code owner February 27, 2020 16:33
ematipico
ematipico suggested changes Feb 27, 2020
View reviewed changes
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 add a test, update the documentation and the snapshot tests

ematipico
ematipico suggested changes Feb 28, 2020
View reviewed changes
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 the way --no-mode is retrieved

packages/webpack-cli/lib/groups/ZeroConfigGroup.js Outdated Show resolved Hide resolved
ematipico
ematipico suggested changes Feb 28, 2020
View reviewed changes
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.

Some other refactor can be done

packages/webpack-cli/__tests__/ZeroConfigGroup.test.js Outdated Show resolved Hide resolved
packages/webpack-cli/lib/groups/ZeroConfigGroup.js Outdated Show resolved Hide resolved
rishabh3112
rishabh3112 requested changes Feb 28, 2020
View reviewed changes
Copy link
Member

@rishabh3112 rishabh3112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting efforts!
Try to open PRs as draft pull requests in case you feel they are work in progress in future 😉.

@snitin315
Copy link
Member Author

snitin315 commented Feb 28, 2020 •
edited

@ematipico @rishabh3112 , Yes there is a bug --mode is accepting any value and outputs development build. I can pass --mode=abcd , and it produces the build.

It should accept only production , development or none

Shoul I open a seperate issue for this? can you please verify?

@ematipico
Copy link
Contributor

Argument validation will be part of another story. Don't worry about it.
Just focus on your feature for now.

@snitin315 snitin315 changed the title feat(webpack-cli): add no-mode flag - [WIP] feat(webpack-cli): add no-mode flag Feb 29, 2020
@ematipico
Copy link
Contributor

Could you rebase your branch please?

@snitin315
Copy link
Member Author

@ematipico already done 👍

ematipico
ematipico suggested changes Mar 1, 2020
View reviewed changes
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.

You also have to update the snapshot tests

@webpack-bot
Copy link

@snitin315 Thanks for your update.

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

@ematipico Please review the new changes.

@snitin315
Copy link
Member Author

@ematipico can you help me fix the CI , dont know why scss test is failing.

@ematipico
Copy link
Contributor

Please add a it.skip to json.test.js. That file is the culprit

@webpack-bot
Copy link

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

@snitin315 snitin315 requested a review from ematipico March 3, 2020 15:02
ematipico
ematipico approved these changes Mar 3, 2020
View reviewed changes
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.

Thank you!

@ematipico ematipico merged commit a069d73 into webpack:next Mar 3, 2020
@snitin315 snitin315 deleted the feat/no-mode-flag branch March 3, 2020 15:11
@snitin315 snitin315 mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico approved these changes

@rishabh3112 rishabh3112 Awaiting requested review from rishabh3112

Assignees
No one assigned
Labels
PR: CI-ok PR: reviewed-approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@snitin315 @ematipico @webpack-bot @rishabh3112