Feature: allow specifying to use before or after other use #42

eliperelman · 2017-10-04T18:15:31Z

No description provided.

edmorley

Great idea!

Have left a few comments below. I was also wondering whether this functionality would be useful for plugins? eg ScriptExtHtmlPlugin must be after htmlTemplate, which in Neutrino it is by default, but with Treeherder we're needing to add multiple extra htmlTemplate, which end up being placed after ScriptExtHtmlPlugin?

edmorley · 2017-10-04T23:01:47Z

README.md

@@ -750,6 +750,50 @@ config.module
      .options({ presets: ['babel-preset-es2015'] });
 ```

+#### Config module rules use before
+
+Specify to that one `use` should be used before another `use` 


Was this meant to be s/to that/that/ ? (And the same below?)

edmorley · 2017-10-04T23:08:51Z

src/Rule.js

+
+      if (_before && uses.includes(_before)) {
+        uses.splice(uses.indexOf(name), 1);
+        uses.splice(Math.max(uses.indexOf(_before), 0), 0, name);


Why is the Math.max() needed? presumably the only time it's required is if uses.indexOf(_before) < 0 (eg -1 for not found), however the uses.includes(_before) presumably rules out that case?

My mistake, this was leftover from a different implementation version.

edmorley · 2017-10-04T23:13:09Z

src/Rule.js

+
+    names.forEach(name => {
+      const { _before, _after } = useEntries[name];
+


Do we need a check to ensure both _before and _after are not set for the same rule, and fail loudly if so? (And a mention in the docs?) Since currently if both are set, the _after clobbers the splices made by the _before handling.

edmorley · 2017-10-04T23:18:35Z

README.md

@@ -750,6 +750,50 @@ config.module
      .options({ presets: ['babel-preset-es2015'] });
 ```

+#### Config module rules use before


The other sections use a naming of form "Config module rules uses (loaders): modifying options" - I don't have a preference as to which to use, but wanted to note that this wasn't consistent with them, in case this wasn't intentional.

eliperelman · 2017-10-05T16:53:46Z

@edmorley Great feedback! In order to make ordering more generic for usage with more than uses, I changed quite a bit of this PR. All the tests should still pass, but now the before and after methods can be applied anywhere we like, and are mergeable as well. Also merging should be smarter and less verbose to perform internally after this.

edmorley

Ah some good cleanup here now too!

I've left some initial comments - though would like to take another look with fresh eyes once it's updated. If it's not too much of a pain to split it up into say 3 commits (one for the variable name changes, one for adding the omit feature, and one for the before/after handling) that would make review a lot easier, especially given I'm less familiar with the codebase. I use SourceTree for easy line by line commit splitting/staging if that's of any use.

This also has conflicts with the README changes on master, could you rebase? :-)

edmorley · 2017-10-05T20:06:36Z

README.md

 // returns: Object, undefined if empty
 entries()
 ````

 ```js
 // Provide an object which maps its properties and values 
-// into the backing Map as keys and values.
+// into the backing Map as keys and values. Can specify
+// an array of keys to omit from the merge.


When I read this I was imagining the keys being omitted might be those on the original object (eg so a new array could be passed in for a particular property and not just merged in with the existing array). However from reading the source below the opposite appears to be the case? (I'm struggling at quick glance to tell though due to the larger diff and not having spent long looking at it) Either way I think it would be good to clarify in this comment :-)

Yeah, I'll try to phrase this better. The omit property is supposed to be an array of keys that should be excluded from merging into the config, e.g. the following would do nothing.

config.merge({ devtool: 'source-map' }, ['devtool'])

edmorley · 2017-10-05T20:07:32Z

README.md

+#### Config plugins: ordering before
+
+Specify that the current `plugin` context should operate before another named `plugin`.
+You cannot use `before()` and `after()` on the same plugin.


s/cannot use/cannot use both/ perhaps? (And in the other instances below)

edmorley · 2017-10-05T20:10:13Z

README.md

+
+config
+  .plugin('html-template')
+    .use(HtmlWebpackTemplate)


The indentation here is inconsistent with some of the other existing entries - this indents the .use() one level deeper than .plugin(), the others don't. Thoughts on which is preferred?

edmorley · 2017-10-05T20:13:57Z

src/ChainedMap.js

+
+  after(name) {
+    if (this.__after) {
+      throw new Error(`Unable to set after(${JSON.stringify(name)}) with existing value for before`);


Should this be after(...) or .after(...) ? (And same for before(...))

I can't decide :-)

edmorley · 2017-10-05T20:15:21Z

src/ChainedMap.js

+
+  merge(obj, omit = []) {
+    if (obj.before && obj.after) {
+      throw new Error(`Unable to merge before: ${JSON.stringify(obj.before)} and after: ${JSON.stringify(obj.after)} on the same object`);


Can this case ever be hit given there are already checks at the time of using before() and after()?

Yes, when merging in configuration from an object, these can be set by the user:

config.merge({ module: { rule: { compile: { use: { cache: { loader: 'cache-loader', before: 'babel' } } } } } })

edmorley · 2017-10-05T20:15:46Z

src/ChainedSet.js

@@ -39,11 +39,11 @@ module.exports = class extends Chainable {
    return this;
  }

-  when(condition, trueBrancher = Function.prototype, falseBrancher = Function.prototype) {
+  when(condition, whenTruthy = Function.prototype, whenFalsy = Function.prototype) {


This could be in a separate cleanup commit in the PR :-)

edmorley · 2017-10-05T20:16:17Z

src/ChainedMap.js

@@ -53,8 +74,23 @@ module.exports = class extends Chainable {
    return this;
  }

-  merge(obj) {
-    Object.keys(obj).forEach(key => this.set(key, obj[key]));
+  merge(obj, omit = []) {


This and the other merge/omit parts could be a separate commit in the PR from the before/after parts :-)

edmorley · 2017-10-05T20:18:40Z

src/Plugin.js

@@ -28,12 +28,12 @@ module.exports = class extends ChainedMap {
      this.set('args', obj.args);
    }

-    return this;
+    return super.merge(obj, [...omit, 'args', 'plugin'])


Missing semicolon.

I wonder if we should add some linting to this repo?

Yeah, I've put it off for a while. I can do that in another PR, although it would be easier to either use Neutrino (seems circular) or move this repo into the monorepo.

edmorley · 2017-10-05T20:21:44Z

test/Rule.js

+    ]
+  });
+});
+


Could you add a test for the "before and after aren't allowed at the same time" handling?

I think we also need some tests for all of the merge omit functionality?

eliperelman · 2017-10-06T00:25:38Z

OK, I think I got everything addressed. Thank you for the in-depth review!

edmorley

Looks great! Thank you for adding the extra tests :-)

edmorley · 2017-10-06T10:30:28Z

README.md

@@ -201,8 +201,11 @@ entries()
 ```js
 // Provide an object which maps its properties and values
 // into the backing Map as keys and values.
+// You can also provide an array to the second argument


s/an array to the second argument/an array as the second argument/ maybe? Up to you :-)

edmorley · 2017-10-06T10:37:00Z

README.md

+#### Config resolve plugins: ordering after
+
+Specify that the current `plugin` context should operate after another named `plugin`.
+You cannot use `.before()` and `.after()` on the same resolve plugin.


s/cannot use/cannot use both/

edmorley · 2017-10-06T10:46:35Z

src/Rule.js

+    if (!omit.includes('oneOf') && 'oneOf' in obj) {
+      Object
+        .keys(obj.oneOf)
+        .forEach(name => this.oneOf(name).merge(obj.oneOf[name]))


Missing semicolon

edmorley · 2017-10-06T10:50:57Z

README.md

@@ -194,6 +193,8 @@ values()
 // where the key is the object property, and the value
 // corresponding to the key. Will return `undefined` if the backing
 // Map is empty.
+// This will order properties by their name if the value is also


s/also// ? Though happy for you to pick

edmorley · 2017-10-06T10:52:48Z

README.md

+
+// Example
+
+config.resolve.plugin('beta')


The non-resolve examples and also the config.resolve "after" example below, put the .plugin() on a new line, so I think this should be too?

edmorley · 2017-10-06T10:58:30Z

src/Orderable.js

+module.exports = (Class) => class extends Class {
+  before(name) {
+    if (this.__after) {
+      throw new Error(`Unable to set .before(${JSON.stringify(name)}) with existing value for after`);


Does the with existing value for after part need quotes or something around the "after", to make it clearer? (Or else .after() ?)

(And same for below)

eliperelman · 2017-10-06T16:48:23Z

Accepted all your requests.

eliperelman · 2017-10-06T16:48:35Z

🎉

eliperelman requested a review from edmorley October 4, 2017 18:15

edmorley suggested changes Oct 4, 2017

View reviewed changes

eliperelman force-pushed the use-before-after branch 4 times, most recently from d397e8f to bcab25a Compare October 5, 2017 16:34

eliperelman requested a review from edmorley October 5, 2017 16:57

eliperelman mentioned this pull request Oct 5, 2017

How to prepend rule loaders? #28

Closed

edmorley suggested changes Oct 5, 2017

View reviewed changes

eliperelman added 2 commits October 5, 2017 19:11

Rename when arguments to be clearer

d15e895

Allow omitting keys from source merge object

fb6ea2f

eliperelman force-pushed the use-before-after branch 2 times, most recently from 6c7ae12 to a9f8f37 Compare October 6, 2017 00:24

eliperelman requested a review from edmorley October 6, 2017 00:25

edmorley approved these changes Oct 6, 2017

View reviewed changes

Feature: allow specifying .before or .after to order plugins and uses

b0040bf

eliperelman force-pushed the use-before-after branch from a9f8f37 to b0040bf Compare October 6, 2017 16:48

eliperelman merged commit 8dc5e34 into neutrinojs:master Oct 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: allow specifying to use before or after other use #42

Feature: allow specifying to use before or after other use #42

eliperelman commented Oct 4, 2017

edmorley left a comment •

edited

Loading

edmorley Oct 4, 2017

edmorley Oct 4, 2017

eliperelman Oct 5, 2017

edmorley Oct 4, 2017

edmorley Oct 4, 2017

eliperelman commented Oct 5, 2017

edmorley left a comment

edmorley Oct 5, 2017

eliperelman Oct 5, 2017

edmorley Oct 5, 2017

edmorley Oct 5, 2017

edmorley Oct 5, 2017

edmorley Oct 5, 2017 •

edited

Loading

eliperelman Oct 5, 2017

edmorley Oct 5, 2017 •

edited

Loading

edmorley Oct 5, 2017

edmorley Oct 5, 2017

eliperelman Oct 5, 2017

edmorley Oct 5, 2017

eliperelman commented Oct 6, 2017

edmorley left a comment

edmorley Oct 6, 2017

edmorley Oct 6, 2017

edmorley Oct 6, 2017

edmorley Oct 6, 2017

edmorley Oct 6, 2017

edmorley Oct 6, 2017

eliperelman commented Oct 6, 2017

eliperelman commented Oct 6, 2017


		names.forEach(name => {
		const { _before, _after } = useEntries[name];


		// Example

		config.resolve.plugin('beta')

Feature: allow specifying to use before or after other use #42

Feature: allow specifying to use before or after other use #42

Conversation

eliperelman commented Oct 4, 2017

edmorley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman commented Oct 5, 2017

edmorley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmorley Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman commented Oct 6, 2017

edmorley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliperelman commented Oct 6, 2017

eliperelman commented Oct 6, 2017

edmorley left a comment •

edited

Loading

edmorley Oct 5, 2017 •

edited

Loading

edmorley Oct 5, 2017 •

edited

Loading