4099 ES6 Refactor lib/Compiler.js #5082
Conversation
webpack-bot
added
PR: CI-ok
PR: tests-needed
PR: CI-not-ok
and removed
PR: CI-not-ok
PR: CI-ok
labels
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
The minimum test ratio has been reached. Thanks! |
lib/Compiler.js
Outdated
| if(err) return self._done(err); | ||
| self.compiler.compile(function onCompiled(err, compilation) { | ||
| _go() { | ||
| const self = this; |
There was a problem hiding this comment.
self was only an alias for this. This is no longer needed with arrow functions
There was a problem hiding this comment.
converted 'self' to 'this' in the _go method. Shifted some normal functions around and converted them to arrow functions.
lib/Compiler.js
Outdated
| var watching = new Watching(this, watchOptions, handler); | ||
| return watching; | ||
| }; | ||
| static Watching(compiler, watchOptions, handler) { |
There was a problem hiding this comment.
That's not the same.
Use Compiler.Watching = Watching; instead.
There was a problem hiding this comment.
moved static Watching method to a property of Compiler class near the last lines
lib/Compiler.js
Outdated
| self.applyPluginsAsync("before-run", self, function(err) { | ||
| if(err) return callback(err); | ||
| run(callback) { | ||
| const self = this; |
There was a problem hiding this comment.
self was only an alias for this. This is no longer needed with arrow functions
There was a problem hiding this comment.
converted 'self' to 'this' in the run method. converted old style function declarations into arrow functions and had to shift position of the function block within the method due to hoisting concerns I think.
lib/Compiler.js
Outdated
| // We can ignore self. | ||
| if(err) return callback(); | ||
| readRecords(callback) { | ||
| const self = this; |
There was a problem hiding this comment.
self was only an alias for this. This is no longer needed with arrow functions
There was a problem hiding this comment.
converted 'self' to 'this' in the readRecords method.
lib/Compiler.js
Outdated
|
|
||
| self.applyPluginsParallel("make", compilation, function(err) { | ||
| compile(callback) { | ||
| const self = this; |
There was a problem hiding this comment.
self was only an alias for this. This is no longer needed with arrow functions
There was a problem hiding this comment.
converted references of self to this in the compile method
|
@KTruong888 Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
test/Compiler.test.js
Outdated
| }); | ||
| // describe("static method", () => { | ||
| // it("should have an accessible static method, Watching", (done) => { | ||
| // const actual = Compiler.Watching(compiler, 1000, err => err); |
There was a problem hiding this comment.
const actual = Compiler.Watching(compiler, 1000, err => err);
const actual = new Compiler.Watching(compiler, 1000, err => err);
There was a problem hiding this comment.
either remove this test or readd it. Don't use commented code.
There was a problem hiding this comment.
Thanks for the review again. Also wondering, would you prefer if I squashed all the commits or just leave it as it is?
webpack-bot
added
PR: reviewed-changes-requested
PR: review-outdated
and removed
PR: review-outdated
PR: reviewed-changes-requested
labels
lib/Compiler.js
Outdated
| }); | ||
| }, | ||
| apply: () => { | ||
| const args = arguments; |
There was a problem hiding this comment.
arguments is not available in an arrow function.
Replace it with a function.
Spread arguments would be nicer, but they are not support in node 4.
…g with arrow functions - reverse timeout change back to normal in NodeWatchFileSystem.test.js
|
Thanks |
What kind of change does this PR introduce?
ES6 Refactor of lib/Compiler.js module
Did you add tests for your changes?
Yes
Summary
Refactored the lib/Compiler.js module to use ES6 as per issue: #4099
Other Notes
I'm also trying to refactor the lib/HotModuleReplacementPlugin and I'm getting a feeling that that file was not meant to be touched because the error messages there are odd. Should I just stop work on that module or is that one meant to be ES6-ified too?
Edit:
Local test suite passed but the checks failed when I pushed. Closed to let others know I am not currently working on this.
edit2:
Tried it again and tests seem to pass now but the test coverage dropped??
edit3:
Added some tests