-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
fix: restore backwards compatibility of ConcatenatedModule (fixes #5477) #5480
fix: restore backwards compatibility of ConcatenatedModule (fixes #5477) #5480
Conversation
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
I couldn't find any tests related to ConcatenatedModule, should I create it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a test
lib/optimize/ConcatenatedModule.js
Outdated
@@ -220,6 +220,10 @@ class ConcatenatedModule extends Module { | |||
} | |||
} | |||
|
|||
get modules() { | |||
return this._orderedConcatenationList.map(m => m.module); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should only include m.type === "concatenated"
modules.
…atenated-module-in-3.5-compat
There was a problem hiding this 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 in a separate PR?
It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff). A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future). @kisenka Please check if this is appliable to your PR and if you can add more test cases. Read the test readme for details how to write test cases. |
Yes, but I don't know where to start. @sokra test for ConcatenatedModule should be like NormalModule.test.js? |
What kind of change does this PR introduce?
Bugfix
Did you add tests for your changes?
I just return an ability to have access to
ConcatenatedModule.modules
.If relevant, link to documentation update:
Summary
Related issue #5477
Does this PR introduce a breaking change?
No
Other information