ES6 refactor lib/node/NodeMainTemplatePlugin.js #5088
Conversation
|
|
lib/node/NodeMainTemplatePlugin.js
Outdated
| this.indent([ | ||
| "installedChunks[chunkId] = [resolve, reject];", | ||
| "var filename = __dirname + " + this.applyPluginsWaterfall("asset-path", JSON.stringify("/" + chunkFilename), { | ||
| hash: "\" + " + this.renderCurrentHashCode(hash) + " + \"", |
|
Thank you for your pull request! The most important CI builds succeeded, we鈥檒l review the pull request soon. |
lib/node/NodeMainTemplatePlugin.js
Outdated
| source, | ||
| "", | ||
| "// uncatched error handler for webpack runtime", | ||
| this.requireFn + ".oe = function(err) {", |
There was a problem hiding this comment.
Could you try to refactor these string concatenations to template strings?
There was a problem hiding this comment.
I've thought about it, but then I found https://jsperf.com/es6-string-literals-vs-string-concatenation/14 and seems like string concatenation is faster than template literals, but template literal is faster than array.join (which is used in Template.asString).
Looking on the code in other modules I couldn't find the prefered way to deal with concatenation - in some places it's + in other places it's template literals.
Do you prefer template literals?
lib/node/NodeMainTemplatePlugin.js
Outdated
| hashWithLength: (length) => "\" + " + this.renderCurrentHashCode(hash, length) + " + \"", | ||
| const request = this.applyPluginsWaterfall("asset-path", JSON.stringify(`./${chunkFilename}`), { | ||
| hash: `" + ${this.renderCurrentHashCode(hash)} + "`, | ||
| hashWithLength: (length) => `" + ${this.renderCurrentHashCode(hash, length)} + \"`, |
There was a problem hiding this comment.
ode(hash, length)} + \"`,
ode(hash, length)} + "`,
lib/node/NodeMainTemplatePlugin.js
Outdated
| hash: "\" + " + this.renderCurrentHashCode(hash) + " + \"", | ||
| hashWithLength: (length) => "\" + " + this.renderCurrentHashCode(hash, length) + " + \"", | ||
| hash: `" + ${this.renderCurrentHashCode(hash)} + "`, | ||
| hashWithLength: (length) => `" + ${this.renderCurrentHashCode(hash, length)} + \"`, |
There was a problem hiding this comment.
rentHashCode(hash, length)} + \"`,
rentHashCode(hash, length)} + "`,
lib/node/NodeMainTemplatePlugin.js
Outdated
| hashWithLength: (length) => "\" + " + this.renderCurrentHashCode(hash, length) + " + \"", | ||
| "var filename = __dirname + " + this.applyPluginsWaterfall("asset-path", JSON.stringify(`/${chunkFilename}`), { | ||
| hash: `" + ${this.renderCurrentHashCode(hash)} + "`, | ||
| hashWithLength: (length) => `" + ${this.renderCurrentHashCode(hash, length)} + \"`, |
There was a problem hiding this comment.
rrentHashCode(hash, length)} + \"`,
rrentHashCode(hash, length)} + "`,
|
It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff). A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future). @ayastreb Please check if this is appliable to your PR and if you can add more test cases. Read the test readme for details how to write test cases. |
|
Thanks |
What kind of change does this PR introduce?
ES6 refactoring
Did you add tests for your changes?
No
If relevant, link to documentation update:
N/A
Summary
Convert codebase to ES6. #4099
Does this PR introduce a breaking change?
No
Other information
My first commit to webpack 馃