Allow array as value in externals object #8043
Conversation
|
|
|
For maintainers only:
|
RubenVerborgh
changed the title
schemas/WebpackOptions.json
Outdated
| @@ -123,6 +123,9 @@ | |||
| { | |||
| "type": "object" | |||
| }, | |||
| { | |||
| "type": "array" | |||
There was a problem hiding this comment.
Be more specific with which items are accepted.
|
@RubenVerborgh Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-approved
PR: reviewed-changes-requested
labels
|
@sokra Done. |
| @@ -0,0 +1,5 @@ | |||
| module.exports = { | |||
| externals: { | |||
| external: ["./math", "subtract"] | |||
There was a problem hiding this comment.
You can use ["webpack", "version"] and check if the result is a string.
Checking the error doesn't really check if subtract is used or ignored.
There was a problem hiding this comment.
I just copied what is in the other test/configCases/externals/optional-externals-*/webpack.config.js tests. These tests are just testing the configuration, I would expect there to be other tests that actually verify the functionality.
There was a problem hiding this comment.
Just tried ["webpack", "version"] and it still errors (test passes without alteration). So if that is supposed to work, there's another issue (which we should then follow up on in #8041 I guess).
There was a problem hiding this comment.
Did you follow the CONTRIBUTION instructions about yarn link && yarn link webpack?
There was a problem hiding this comment.
Right, I didn't, as this was originally just a config change 😄
Thanks for the guidance, will fix.
There was a problem hiding this comment.
Followed the instructions, same result. Looks like CI failed due to a random timeout (only AppVeyor failed). Will push commit again.
webpack-bot
added
PR: reviewed-changes-requested
PR: CI-not-ok
and removed
PR: review-outdated
PR: CI-ok
labels
webpack-bot
added
PR: review-outdated
PR: CI-ok
and removed
PR: reviewed-changes-requested
PR: CI-not-ok
labels
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Thanks |
|
btw. your test didn't work, because the Array version wasn't really working internally. It works now. |
|
@sokra Yeah that's what I figured. Thanks for fixing. Good that you insisted on a functionality test. |
Fixes #8041.
#8041 (comment)
What kind of change does this PR introduce?
bugfix (schema)
Did you add tests for your changes?
Yes.
Does this PR introduce a breaking change?
No.
What needs to be documented once your changes are merged?
The documentation is alright; it already mentions this option:
https://webpack.js.org/configuration/externals/#array
(but that doesn't work yet without this PR)