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

fix: allow array of strings for output.library.root #7184

Merged
merged 3 commits into from
May 12, 2018
Merged

fix: allow array of strings for output.library.root #7184

merged 3 commits into from
May 12, 2018

Conversation

byzyk
Copy link
Member

@byzyk byzyk commented May 3, 2018 •
edited

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

configCases

If relevant, link to documentation update:

N/A

Summary

Closes #7176

Does this PR introduce a breaking change?

No

@byzyk
Copy link
Member Author

byzyk commented May 3, 2018

I assume tests will be needed for this one

@webpack-bot
Copy link
Contributor

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

sokra
sokra requested changes May 3, 2018
View reviewed changes
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Add a testcase in test\configCases

@montogeek
Copy link
Member

Isn't needed a change in the source code?

@byzyk
Copy link
Member Author

byzyk commented May 3, 2018

@montogeek quite possible. Need to add a test case first and will see, probably tom

@webpack-bot
Copy link
Contributor

@byzyk Thanks for your update.

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

@sokra Please review the new changes.

montogeek
montogeek reviewed May 4, 2018
View reviewed changes
test/configCases/library/umd/webpack.config.js Outdated
root: "testLibrary",
amd: "test-library",
commonjs: "test-library"
module.exports = [
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it is fine to edit an existing test.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are quite similar so I thought they can live in the same folder. But it's not a big deal I can create a separate case. Interesting that I did exactly that in the first place but then I thought why not together? 😃

@byzyk
Copy link
Member Author

byzyk commented May 5, 2018

Added a test case. Seems it compiles without errors

@byzyk byzyk mentioned this pull request May 5, 2018
Closed
@byzyk
Copy link
Member Author

byzyk commented May 10, 2018

I checked with the issue author and he confirmed it works as expected. Is there anything else pending to get this one merged?

@sokra sokra merged commit d5a648b into webpack:master May 12, 2018
@sokra
Copy link
Member

sokra commented May 12, 2018

Thanks

@byzyk byzyk deleted the fix/allow-array-in-lib-root branch May 14, 2018 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ooflorent ooflorent ooflorent approved these changes

@montogeek montogeek montogeek approved these changes

@sokra sokra sokra approved these changes

@TheLarkInn TheLarkInn TheLarkInn approved these changes

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

Successfully merging this pull request may close these issues.

output.library.root should accept an array
6 participants
@byzyk @webpack-bot @montogeek @sokra @ooflorent @TheLarkInn