Only use UMD or UMD2 externals for UMD libraryTarget. Resolve #5766#5784
Only use UMD or UMD2 externals for UMD libraryTarget. Resolve #5766#5784sokra merged 1 commit intowebpack:masterfrom NMinhNguyen:bugfix/only-use-umd-externals-for-umd-library-target
Conversation
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
sokra
left a comment
There was a problem hiding this comment.
Add a test case -> test/configCases
|
@NMinhNguyen Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
@sokra Thanks for the review. I've added two test cases: one for UMD and one for UMD2. Please let me know if you require further changes (e.g. if the tests aren't quite right or you'd like to see more tests), I'll happily make them 😃 |
|
@sokra I've noticed that coverage decreased despite the tests, do you think it's something to worry about? I'm quite worried about this change having unintended side effects 😨 Update: the coverage thing may have been before I added tests? Seems back to normal now 😅 |
|
It takes a while until all optional CI test finished. This causes coverage report to be incorrect for some time. |
What kind of change does this PR introduce?
Depending on if the issue I raised can be considered a bug, this is either a bugfix or a feature 😃
This change should remove
require/definestatements, whose output does not end up being used when using non-UMD externals (such asvar) as seen in the diff here: NMinhNguyen/webpack-umd-with-var-externals-example@f0126c5Did you add tests for your changes?
No,
as there weren't any existing tests for, but I'm happy to add them if necessary - I realise this is quite an important part of the code. All existing tests pass though.UmdMainTemplatePluginUpdate: I was looking for a
UmdMainTemplatePlugin.test.jsfile but I now realise this code is actually tested viaConfigTestCases- I need to familiarise myself with the existing tests but I fully intend on converting my repro into a test case, at the very least.If relevant, link to documentation update:
Summary
Resolves #5766
Does this PR introduce a breaking change?
I don't believe it does - if anything, this should potentially prevent breakages?
Other information
@sokra suggested checking for
m.type !== "umd" && m.type !== "umd2", but I went form.type === "umd" || m.type === "umd2"(the logical opposite of the suggested change)