Fix the exception thrown when writeToDisk is enabled in multi-compiler mode
#301
Conversation
|
|
|
@arianrhodsandlot Good work thanks! |
Codecov Report
@@ Coverage Diff @@
## master #301 +/- ##
==========================================
+ Coverage 96.81% 96.83% +0.02%
==========================================
Files 7 7
Lines 251 253 +2
==========================================
+ Hits 243 245 +2
Misses 8 8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for the PR 🍺
Unfortunately this is a less than optimal solution, and it can't be accepted as-is. Dealing with multi-compilers should written as: https://github.com/webpack-contrib/webpack-hot-client/blob/0d8707da129cdb6d9b892603c330f4dd2eac829f/index.js#L123-L126. And that would mean theres no need for a separate function (and no really long function names 🎉), as the code can simply be wrapped in the for/of. That'll also mean that there's less of a drop in coverage.
|
@shellscape Brilliant suggestion. I've changed my code 😸. |
| module.exports = [{ | ||
| mode: 'development', | ||
| context: __dirname, | ||
| entry: './foo.js', | ||
| output: { | ||
| filename: 'foo.js', | ||
| path: '/js1', | ||
| path: path.resolve(__dirname, 'js1'), |
There was a problem hiding this comment.
curious: previous tests pass on this with the path.resolve. is there a reason for this change?
There was a problem hiding this comment.
As documented, output.path should be an absolute path. And without path.resolve, it will attempt to write files into the root directory /. This configuration didn't cause an error in the past because files are all in memory when no test cases with writeToDisk: trueare using it.
test/tests/server.js
Outdated
| @@ -377,4 +377,49 @@ describe('Server', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| function writeToDiskWithMultiCompiler(value, done) { | |||
There was a problem hiding this comment.
maintainer's preference here: please do shorten this function name. something under 15 characters preferably, e.g. multiToDisk
|
Note: we can ignore the coverage report on this PR. not sure why it labeled it as a 0.02% reduction on last commit |
|
Hi!! first of all thanks for the great job. Do you know when this will be release? |
|
@Dagniele when we're able to publish it. patience is a virtue, and given the PR was merged under 24 hours ago, your followup there is a bit premature. |
|
@shellscape no problem, I was not impatience or complaining I just wanted to know if you had any plan or schedule 😄 |
What kind of change does this PR introduce?
This is a bugfix 🐛🔫 for #290 😸. I'm running into the same problem these days.
Did you add tests for your changes?
Yes, I've added a test in
test/tests/server.jsSummary
Fix the exception thrown when
writeToDiskis enabled in multi-compiler mode.Does this PR introduce a breaking change?
No.