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

feat(webpack-cli): add mode argument validation #1290

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Mar 4, 2020

What kind of change does this PR introduce?

Fixes #1282

Refers #1261

Did you add tests for your changes?
Yes.

If relevant, did you update the documentation?
Yes

Summary
Added validation for mode argument , now can only accept development , production or none
will promt a warning if set to any other value.

Does this PR introduce a breaking change?

NO.

@snitin315 snitin315 requested a review from a team as a code owner March 4, 2020 13:39
@webpack-bot
Copy link

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

packages/webpack-cli/lib/utils/cli-flags.js Outdated Show resolved Hide resolved
test/mode/prod/prod.test.js Outdated Show resolved Hide resolved
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 change the test. I we should not say anything about falling back. Webpack already does that and it prints a warning too.

packages/webpack-cli/lib/utils/cli-flags.js Outdated Show resolved Hide resolved
@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 snitin315 force-pushed the feat/mode-validation branch 2 times, most recently from 6df4c94 to 430c6ff Compare March 4, 2020 16:52
@snitin315 snitin315 requested a review from ematipico March 4, 2020 17:02
@snitin315
Copy link
Member Author

snitin315 commented Mar 5, 2020

I think we should add the test case for invalid value , as now we are returning production in this case.

@ematipico what do you say?

@ematipico
Copy link
Contributor

Yes of course.
We still have a failing message about the snapshot. It's about your previous pr. Could you check that? I believe the issue is windows ending line

@ematipico
Copy link
Contributor

@snitin315 I fixed the failing tests. Would you please rebase your PR?

@snitin315
Copy link
Member Author

@ematipico Done 💯

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.

We're almost there!

test/mode/prod/prod.test.js Show resolved Hide resolved
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!

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.

bug(cli): mode flag accepts any value
4 participants