Refactor NormalModule to es6 by timse · Pull Request #4259 · webpack/webpack

timse · 2017-02-11T06:13:29Z

What kind of change does this PR introduce?

refactor

Did you add tests for your changes?

N/A

If relevant, link to documentation update:

N/A

Summary

refactor NormalModule to es6

Does this PR introduce a breaking change?

No

Other information

Some refactorings to split up bigger method bodies in smaller ones. (to make it easier to read for stupid people like me :D )

- also remove unused param - call callback outside of method call

…module" variable

sokra

Looks good

sokra · 2017-02-12T11:20:03Z

+				module._compile(code, filename);
+				return module.exports;
+			},
+			resolve: function(context, request, callback) {


arrow functions

sokra · 2017-02-12T11:27:18Z

+			return this.applyNoParseRule(noParseRule, request);
+		}
+
+		for(let i = 0; i < noParseRule.length; i++) {


Please add a tests for this. It's new and not tested.

sokra · 2017-02-12T11:29:52Z

+		super.disconnect();
+	}
+
+	markModuleAsErrored() {


Could you rewrite this method to take an error argument and be the only place where this.error = error is set? This should reduce the number of places where this.error is written and make the control flow more clear.

sokra · 2017-02-12T11:30:22Z

+	}
+
+	source(dependencyTemplates, outputOptions, requestShortener) {
+		let hash = require("crypto").createHash("md5");


require crypto on top of the his file.

sokra · 2017-02-12T11:33:07Z

+					if(varNames.length === 0) return;
+					varStartCode += "/* WEBPACK VAR INJECTION */(function(" + varNames.join(", ") + ") {";
+					// exports === this in the topLevelBlock, but exports do compress better...
+					varEndCode = (topLevelBlock === block ? "}.call(" + (topLevelBlock.exportsArgument || "exports") + ", " : "}.call(this, ") +


Could you make this line more readable? i. e. by moving some parts into local variables.

sokra · 2017-02-12T11:34:41Z

+	needRebuild(fileTimestamps, contextTimestamps) {
+		const highestFileDepTimestamp = this.getHighestTimestamp(
+			this.fileDependencies, fileTimestamps);
+		// if the hightest is Infinity, we need a needRebuild


we need a needRebuild -> we need a rebuild

timse · 2017-02-12T13:11:07Z

will do the tasks tomorrow! :)
looks like i missed out on the entire source method.
Will take a closer look at it!

tasks:

…s called with an error

sokra · 2017-02-15T12:54:29Z

+	// check if module should not be parsed
+	// returns "true" if the module should !not! be parsed
+	// returns "false" if the module !must! be parsed
+	preventParsing(noParseRule, request) {


preventParsing -> isParsingPrevented

sokra · 2017-02-15T12:55:30Z

+	}
+
+	variableInjectionFunctionWrapperStartCode(varNames) {
+		const openingIIFEParanthesis = "(";


This is an overkill. Just add the ( to the string and add a comment if you want to make it clear.

sokra · 2017-02-15T12:55:38Z

+	variableInjectionFunctionWrapperEndCode(varExpressions, block) {
+		const firstParam = this.contextArgument(block);
+		const furtherParams = varExpressions.map(e => e.source()).join(", ");
+		const closingIIFEParanthesis = ")";


sokra · 2017-02-15T13:04:09Z

+	}
+
+	splitVariablesInUniqueNamedChunks(vars) {
+		const chunkCollection = vars.reduce((chunkCol, variable) => {


This can be simplied to not require a tempChunk.

return vars.reduce((chunks, variable) => { const current = chunks[chunks.length - 1]; if(current.some(v => v.name === variable.name)) { chunks.push([variable]); } else { current.push(variable); } return chunks; }, [[]]);

fair enough

sokra · 2017-02-15T13:04:31Z

+			source: source,
+			hash: hashDigest
+		};
+		const topLevelBlock = this;


remove this line

just realized that even the parameter this resembles doesnt even exist :)

sokra · 2017-02-15T13:05:37Z

+					name: "baz"
+				},
+				{
+					name: "wurst"


use english names.

sokra · 2017-02-15T13:06:29Z

+		loaders = [];
+		resource = "some/resource";
+		parser = {
+			parser() {}


This is weird. A Parser doesn't have a parser method. I would expect constructor() {} or parse() {}.

this was supposed to be parse! nice catch

sokra · 2017-02-15T13:06:59Z

+		describe("given a noParseRule", function() {
+			let returnValOfSpy;
+			beforeEach(function() {
+				returnValOfSpy = Math.random() >= 0.5 ? true : false;


Never use Math.random in tests. They should be deterministic.

i kind of wanted to make them be random, but i guess that doesnt help should one ever try to debug such a test. will change

sokra · 2017-02-15T13:07:04Z

+					let someRules;
+					beforeEach(function() {
+						someRules = [
+							Math.random() >= 0.5 ? "some rule" : /some rule/,


Never use Math.random in tests. They should be deterministic.

sokra · 2017-02-15T13:08:19Z

+		beforeEach(function() {
+			variables = [{
+					name: "foo"
+				},


The array styling looks weird.

The following should be accepted by the linter:

variables = [{ name: "foo" }, { name: "bar" }]

it does look weird
but if i do what you suggest (what i tried in the first place) the linter complains.
Having it not like that makes eslint complain though.
The two linters should be aligned at some point

+++ test/NormalModule.test.js @@ -245,22 +245,22 @@ describe("#splitVariablesInUniqueNamedChunks", function() { let variables; beforeEach(function() { variables = [{ - name: "foo" - }, - { - name: "bar" - }, - { - name: "baz" - }, - { - name: "some" - }, - { - name: "more" - } + name: "foo" + }, + { + name: "bar" + }, + { + name: "baz" + }, + { + name: "some" + }, + { + name: "more" + } ]; });

Try it with }, { in between. See my above example.

sokra · 2017-02-16T10:03:33Z

Thanks

timse added 12 commits February 11, 2017 14:16

migrate NormalModule to es6

f16867d

split up "updateHash" into subtask methods

c9038ac

fixme

c948f18

rename setError to "markModuleAsErrored" and move it to prototype

e028b89

- also remove unused param - call callback outside of method call

extract loadercontext creation to its own method

a766b66

extract source creation to its own method

d4c4853

use arrow function to bind this and remove indirection over local "…

2a66bb4

…module" variable

use const for source, as let is no longer necessary

128e043

refactor needRebuild to make use of early return if possible

93377f7

split out noParse check for modules

1e4d929

extract asset source creation to own method

7d36c5b

fix typo

80e7d40

sokra requested changes Feb 12, 2017

View reviewed changes

sokra added the Refactor (ES5 => ES6) label Feb 12, 2017

apply review tasks

b9507cc

timse added 8 commits February 13, 2017 21:38

simplify error handling and simplify markModuleAsErrored as its alway…

a5d5b9e

…s called with an error

untangle source method

4d378f5

refactor variable cleanup code generation, step one

fcac9a3

refactor part 2, add comments

c71840a

add tests for NormalModule

eb2d6d6

add tests for preventParsing and applyNoParseRule

3812c10

fix style issues

a7bbe58

skip the windows path test for now

b2b7021

sokra requested changes Feb 15, 2017

View reviewed changes

timse added 2 commits February 16, 2017 08:01

apply review tasks

c06b066

fix test and linting

db26265

sokra approved these changes Feb 16, 2017

View reviewed changes

sokra merged commit 27deabc into webpack:master Feb 16, 2017

timse deleted the refactor-normalmodule-to-es6 branch February 16, 2017 10:23

Uh oh!

Conversation

timse commented Feb 11, 2017

Uh oh!

sokra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timse commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra commented Feb 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

timse commented Feb 12, 2017 •

edited

Loading