FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: support both webpack versions by evenstensberg · Pull Request #1065 · webpack/webpack-cli · GitHub

/ webpack-cli Public

fix: support both webpack versions - #1065

Merged
evenstensberg merged 5 commits into
masterfrom
fix/patch
Sep 5, 2019
ConversationCommits (5)ChecksFiles changed
Merged

fix: support both webpack versions#1065
evenstensberg merged 5 commits into
masterfrom
fix/patch

Conversation

evenstensberg commented Aug 26, 2019

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?
#1023
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
N/A
Summary

Tries to resolve validation for both versions.

Does this PR introduce a breaking change?
no

Other information

@sokra could you verify? Tested locally, removed the if/else but you might be aware of anymore pitfalls.

sokra approved these changes Aug 26, 2019
View reviewed changes

sokra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me, haven't tried to run it

ematipico reviewed Aug 27, 2019
View reviewed changes

ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code wise looks good. I would move it into a separate a function so we can test it

Comment thread bin/utils/convert-argv.js Outdated
const error = new WebpackOptionsValidationError(webpackConfigurationValidationErrors);
console.error(error.message, `\nReceived: ${typeof options} : ${JSON.stringify(options, null, 2)}`);
process.exit(-1); // eslint-disable-line
let error;

ematipico Aug 27, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you make a function out of it and export it? So we can test it and make sure 100% that it works.

evenstensberg Aug 27, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yup will do

webpack-bot commented Aug 28, 2019

Copy link
Copy Markdown

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

willgriffiths mentioned this pull request Sep 3, 2019
Closed
16 tasks
ematipico suggested changes Sep 4, 2019
View reviewed changes

ematipico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We should add the test now

webpack-bot commented Sep 5, 2019

Copy link
Copy Markdown

@evenstensberg Thanks for your update.

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

@ematipico Please review the new changes.

evenstensberg commented Sep 5, 2019

Copy link
Copy Markdown
Member Author

Thanks for reviewing @ematipico . Singing this of and releasing 💙

evenstensberg merged commit 64fd810 into master Sep 5, 2019
evenstensberg deleted the fix/patch branch September 5, 2019 16:54
willgriffiths mentioned this pull request Sep 11, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

sokra sokra approved these changes
+1 more reviewer
ematipico ematipico requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

PR: CI-ok PR: review-outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL