-
-
Notifications
You must be signed in to change notification settings - Fork 257
Conversation
Codecov Report
@@ Coverage Diff @@
## master #183 +/- ##
========================================
+ Coverage 96.96% 100% +3.03%
========================================
Files 2 2
Lines 33 35 +2
Branches 16 16
========================================
+ Hits 32 35 +3
+ Misses 1 0 -1
Continue to review full report at Codecov.
|
src/cjs.js
Outdated
@@ -1 +1,4 @@ | |||
module.exports = require('./index').default; | |||
const fileLoader = require('./index'); |
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.
fileLoader
=> loader
😛
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.
@michael-ciniawsky 👍 done
raw
value
src/cjs.js
Outdated
const fileLoader = require('./index'); | ||
|
||
module.exports = fileLoader.default; | ||
module.exports.raw = fileLoader.raw; |
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 is mutating fileLoader.default
... 😕
Maybe it could be better to write the workaround in index.js
itself, with a comment that it's a workaround for the commonjs export.
They'll both do the same thing, and I guess technically put the cjs change outside of the cjs wrapper, but I'd prefer that to being mutated from outside.
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.
@Kovensky Perhaps, but on the other hand, I would not want to add workarounds to the loader code, because it can be misleading. In any case, I'm open to ideas, I'll be glad to see another solution
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.
@Kovensky What are potential issues with mutating loader.default
? Handling the Module System related stuff in the wrapper would be 👍 if possible 🙃
fb94f98
to
8d1e15f
Compare
@michael-ciniawsky just ensure tests are good, also npm have bug with |
Use hotfixed file-loader for now, until webpack-contrib/file-loader#183 is released.
@evilebottnawi See #204 for a fix to tests. |
@brianhelba This needs to be updated in @d3viant0ne @evilebottnawi If it is the only known failure blocking, can we merge the fix regardless of the npm CI incompat for now and fix it in a follow up PR? Setting |
@michael-ciniawsky I agree, I'd like to merge this PR ASAP, even without the fix in #204. The CI tests are already failing on @d3viant0ne @evilebottnawi Is that ok? |
7d7ec84
to
f9b839e
Compare
Excellent! Thanks for the quick attention and release. |
We use
"main": "dist/cjs.js"
inpackage.json
, butdefault
(https://github.com/webpack-contrib/file-loader/blob/master/src/cjs.js#L1) doesn't containraw
value sofile-loader
doesn't work correctly.