refactor(es6): refactor LoaderOptionsPlugin to ES6 class by carloscuatin · Pull Request #3657 · webpack/webpack

carloscuatin · 2016-12-31T01:11:59Z

What kind of change does this PR introduce?

Refactor (ES5 => ES6)

Did you add tests for your changes?

No functionality was changed, existing tests passed

If relevant, link to documentation update:

Not

Summary

Refactors the LoaderOptionsPlugin to an ES6 class. First time contributor 🎉

Does this PR introduce a breaking change?

Not

Other information

jsf-clabot · 2016-12-31T01:12:00Z

All committers have signed the CLA.

Jessidhia · 2016-12-31T02:20:07Z

+						return ["include", "exclude", "test"].indexOf(key) < 0
+					}).forEach((key) => {
+						context[key] = options[key];
+					});


Hm, I wonder; what is the preferred syntax for arrow with single argument:

key => ["include", "exclude", "test"].indexOf(key) < 0

(key) => ["include", "exclude", "test"].indexOf(key) < 0

This is assuming, of course, the single-expression return is allowed... and it'd be a shame IMO if it wasn't :P

I also wonder what's the preferred syntax for chaining (not sure there's an eslint rule for this):

// single-line; any newlines are part of the argument lists Object.keys(options).filter(...).forEach(...);

// dot-first Object.keys(options) .filter(...) .forEach(...);

// dot-first indented Object.keys(options) .filter(...) .forEach(...);

Looks like a combination of dot-location and newline-per-chained-call can specify the last 2 examples, but no way to control the indent (both are considered valid).

Couldn't find a rule to enforce single line.

@Kovensky thanks for the review 👍 , for arrow is single line

(key) => ["include", "exclude", "test"].indexOf(key) < 0

for chaining i prefer

Object.keys(options) .filter(...) .forEach(...);

Jessidhia · 2016-12-31T02:31:00Z

+				const i = resource.indexOf("?");
+				if(ModuleFilenameHelpers.matchObject(options, i < 0 ? resource : resource.substr(0, i))) {
+					Object.keys(options).filter((key) => {
+						return ["include", "exclude", "test"].indexOf(key) < 0


This could be hoisted into a const filterSet = new Set(["include", "exclude", "test"]) (next to the const options), so the filter can be simplified into .filter(key => !filterSet.has(key)). Basically, anytime you see a membership test, it almost always can be converted into a Set.

I also remember having seen something in another PR about wanting "object"-typed values using let instead of const, but I don't understand the reasoning...

We can ignore the declatation type for now. We don't have an eslint style for it now. I'll probably convert them all to const once we get closer.

Jessidhia · 2016-12-31T08:42:50Z

+			compilation.plugin("normal-module-loader", (context, module) => {
+				let resource = module.resource;
+				if(!resource) return;
+				let i = resource.includes("?");


String.prototype.includes is not available on Node 4

@Kovensky Would a polyfill not apply in this case?

Nope; webpack doesn't seem to be using any polyfills...

let i = resource.indexOf("?") !== -1

willmendesneto · 2017-01-02T22:23:26Z

-				});
-			}
+	apply(compiler) {
+		let options = this.options;


In this case we can use const, because options doesn't change.

const options = this options

I'm fine with this for now. I'm going to run eslint --fix when we get to a good point after adding prefer-const.

willmendesneto · 2017-01-02T22:24:23Z

+				let i = resource.includes("?");
+				if(ModuleFilenameHelpers.matchObject(options, !i ? resource : resource.substr(0, i))) {
+					Object.keys(options)
+						.filter((key) => !["include", "exclude", "test"].includes(key))


.filter((key) => !["include", "exclude", "test"].indexOf(key) !== -1

Array.prototype.includes is fine in Node 4.

I do still want this to be a const Set instead of array, though... (#3657 (comment))

TheLarkInn

@carloscuatin way to go choosing this as your first ever webpack PR.

For this first review, lets just get the tests passing.

So looks like remove includes and use something node 4 compatible like Set or [1,2,4,5].indexOf(5) > 0 instead.

carloscuatin · 2017-01-03T23:15:07Z

@TheLarkInn Thanks, I made the arrangements

TheLarkInn · 2017-01-04T05:12:44Z

-				});
-			}
+	apply(compiler) {
+		let options = this.options;


I'm fine with this for now. I'm going to run eslint --fix when we get to a good point after adding prefer-const.

TheLarkInn · 2017-01-04T05:13:19Z

Awesome work. Congrats on merging your first webpack PR 🎉 🍾

* refactor(es6): refactor LoaderOptionsPlugin to ES6 class

refactor(es6): refactor LoaderOptionsPlugin to ES6 class

5297b9f

Jessidhia reviewed Dec 31, 2016

View reviewed changes

carloscuatin added 2 commits December 30, 2016 22:54

fixes in arrow function single argument and syntax for chaining

eea1277

replace indexOf for includes

7d7d1f0

TheLarkInn added Refactor (ES5 => ES6) enhancement labels Dec 31, 2016

Jessidhia reviewed Dec 31, 2016

View reviewed changes

willmendesneto reviewed Jan 2, 2017

View reviewed changes

TheLarkInn requested changes Jan 3, 2017

View reviewed changes

TheLarkInn added PR: direction fine and removed PR: investigating if direction is fine labels Jan 3, 2017

fix recommended by the reviews

cd818c7

TheLarkInn approved these changes Jan 4, 2017

View reviewed changes

TheLarkInn merged commit fbeb8ca into webpack:master Jan 4, 2017

timse pushed a commit to timse/webpack that referenced this pull request Jan 4, 2017

refactor(es6): refactor LoaderOptionsPlugin to ES6 class (webpack#3657)

1ec702b

* refactor(es6): refactor LoaderOptionsPlugin to ES6 class

Uh oh!

Conversation

carloscuatin commented Dec 31, 2016

Uh oh!

jsf-clabot commented Dec 31, 2016 • 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!

carloscuatin Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jessidhia Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

TheLarkInn left a comment

Choose a reason for hiding this comment

Uh oh!

carloscuatin commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheLarkInn commented Jan 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jsf-clabot commented Dec 31, 2016 •

edited

Loading

carloscuatin Dec 31, 2016 •

edited

Loading

Jessidhia Dec 31, 2016 •

edited

Loading