-
-
Notifications
You must be signed in to change notification settings - Fork 385
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: enforce esm #546
fix: enforce esm #546
Conversation
Codecov Report
@@ Coverage Diff @@
## master #546 +/- ##
==========================================
+ Coverage 88.49% 88.55% +0.05%
==========================================
Files 5 5
Lines 426 428 +2
Branches 94 96 +2
==========================================
+ Hits 377 379 +2
Misses 47 47
Partials 2 2
Continue to review full report at Codecov.
|
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.
Can we add test?
we can.. doesn't know what to expect from it since difference would be only in size |
@vankop we can just snapshot module (from |
@@ -6,9 +6,11 @@ import { createFsFromVolume, Volume } from 'memfs'; | |||
import MiniCssExtractPlugin from '../../src'; | |||
|
|||
export default (fixture, loaderOptions = {}, config = {}) => { | |||
const { outputFileSystem, ...cnfg } = config; |
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.
Not related to PR fix, I just wanted to look on output when was testing locally and found a bug with outputFileSystem
option..
@evilebottnawi damn.. we need upgrade Node.js version to test with webpack@5 .. |
@vankop why? We use |
@evilebottnawi I added webpack@5 (take a look on PR) and it fails with Node.js v8.x in CI |
You don't need to add |
} | ||
); | ||
const stats = await compile(compiler); | ||
expect(stats.hasErrors()).toBe(false); |
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.
export default (name, stats) => {
const { modules } = stats.toJson({ source: true });
const module = modules.find((m) => m.name === name);
let { source } = module;
return source;
};
Just snapshot module using name
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.
fixed, let me clean up and squash commit..
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.
Can we merge?
@evilebottnawi yes, looks good |
This PR contains a:
Motivation / Use-Case
Enforce esm for empty module, so webpack can make some optimizations.
Breaking Changes
No
Additional Info
related issue webpack/webpack#10889