-
-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix(modern-compiler): dispose redundant compilers #1245
fix(modern-compiler): dispose redundant compilers #1245
Conversation
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 sign the CLA?
Sure, signed it. |
@@ -768,6 +768,8 @@ function getCompileFn(loaderContext, implementation, apiType) { | |||
webpackCompiler.hooks.shutdown.tap("sass-loader", () => { | |||
compiler.dispose(); | |||
}); | |||
} else { | |||
compiler.dispose(); |
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.
I'm not a fan of if/elses deeply nested, but we can't return early in this scope, so it's okay.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1245 +/- ##
==========================================
- Coverage 94.44% 94.23% -0.22%
==========================================
Files 3 3
Lines 360 416 +56
Branches 132 158 +26
==========================================
+ Hits 340 392 +52
- Misses 18 22 +4
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Seems like the coverage is failing here. Could you add a test for this? |
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.
I was wrong
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.
I will add a test case
004ed38
into
webpack-contrib:master
This PR contains a:
idk how to cover this by tests
Motivation / Use-Case
fixes #1244
Breaking Changes
No
Additional Info
No