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

allow array value for --ouput-library #559

Merged
merged 2 commits into from
Aug 2, 2018

Conversation

connorjclark
Copy link
Contributor

@connorjclark connorjclark commented Aug 1, 2018

Fixes #557.

@connorjclark connorjclark changed the title allow array value for --ouput-library. Fixes #557 allow array value for --ouput-library Aug 1, 2018
@ematipico
Copy link
Contributor

Hi @hoten, thanks for your PR!
I wonder if you could add another test where we pass just one argument as --output-library? To make sure we don't break existing functionality (not tested, as far as I can see).

@@ -441,7 +441,10 @@ module.exports = function(...args) {

ifArg("output-library", function(value) {
ensureObject(options, "output");
options.output.library = value;
if (typeof options.output.library === "undefined") {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can do if (!options.output.library) here

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Could you add a test for the thing @ematipico commented on?

@webpack-bot
Copy link

@hoten Thanks for your update.

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

@ev1stensberg Please review the new changes.

@connorjclark
Copy link
Contributor Author

Done.

@webpack-bot
Copy link

@hoten The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from travis (failure) and fix these issues.

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.

Thanks!

@dhruvdutt dhruvdutt merged commit 6014ec8 into webpack:master Aug 2, 2018
evenstensberg added a commit that referenced this pull request Aug 2, 2018
evenstensberg added a commit that referenced this pull request Aug 2, 2018
@evenstensberg
Copy link
Member

Hi @hoten , could you re-open in a new PR? Things like this needs a better review before merging. @dhruvdutt shouldn't really merge PR's that are unapproved by all members as he is a student for us during the summer..

@dhruvdutt
Copy link
Member

My bad. Let's take a thorough relook. Can you please open a new PR @hoten?

@connorjclark
Copy link
Contributor Author

Opened a new PR.

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.

5 participants