Neutrino refactor #119

jaridmargolin · 2017-03-16T01:44:54Z

Tried to break these up into nice little reviewable commit chunks.

1. Move require preset functionality to api.
The idea here is to expose the requirePresets functionality for reuse by anyone wishing to consume the api. I don't see any specific reason why it would be specific to the CLI. Any oversight?

2. Make run method in bin/neutrino self contained.
This one may be stylistic, but I didn't understand why some variables were declared in the method, and some were declared at the top of the file. This moves all of the variables to where they are actually utilized. Additionally, I believe this has the added benefit of making run more portable if in the future there is reason to have it usable outside of the CLI.

3. Move run process logic to location of call.
The moves the logging and process management to the location in which run is called. I believe this is more appropriate place and makes the code easier to reason about... The idea is, "this is what we do when run fails." Additionally this closes, #118.

* Hopefully not out of line making structural changes to the code. I have very much enjoyed working with the neutrino community, and thought I could give back a little.

jaridmargolin · 2017-03-16T01:55:20Z

Additionally, I didn't see tests built around the CLI functionality. Any insight on how you are currently working with this code and maintaining confidence in the changes?

eliperelman · 2017-03-16T14:00:55Z

@jaridmargolin I typically don't test CLI-specific functionality as it usually involves spawning a process and reading its output. I'll typically stick with testing APIs. If someone wanted to add a PR for a way to test the CLI that was simple enough, I would accept it.

eliperelman · 2017-03-16T14:01:59Z

packages/neutrino/bin/neutrino

@@ -10,17 +10,8 @@ const { pathOr, pipe, partialRight } = require('ramda');
 const { join } = require('path');
 const stringify = require('javascript-stringify');
 const sort = require('deep-sort-object');
+const optional = require('optional');


Oooo, never seen optional before, very cool!

eliperelman · 2017-03-16T14:03:20Z

packages/neutrino/bin/neutrino

  const options = pathOr({}, ['neutrino', 'options'], pkg);
  const config = pathOr({}, ['neutrino', 'config'], pkg);
  const api = new Neutrino(Object.assign(options, { config }));
+  const inspect = pipe(sort, partialRight(stringify, [null, 2]), console.log, process.exit);


Let's move inspect above api so the flow to Neutrino is clear.

eliperelman · 2017-03-16T14:06:07Z

packages/neutrino/bin/neutrino

-      `Ensure this module can be found relative to the root of the project.`);
-  });
+  // Grab all presets and merge them into a single webpack-chain config instance
+  api.requirePresets(presets)


Let's either one-line this or put each chain on its own line.

api .requirePresets(presets) .forEach(preset => api.use(preset));

eliperelman · 2017-03-16T14:07:25Z

packages/neutrino/bin/neutrino

-
-      process.exit(1);
-    });
+  return args.inspect


Put ternary operators on the preceding line:

return args.inspect ? inspect(api.getWebpackOptions()) : api[command](args);

eliperelman · 2017-03-16T14:08:58Z

packages/neutrino/bin/neutrino

 }

-run(args._[0], [...new Set(pkgPresets.concat(args.presets))]);
+run(args._[0], args)


There's only a single method call here, so we can probably put this one on the same line:

run(args._[0], args).catch(err => { console.error(err || `Error during ${args._[0]}`); process.exit(1); });

I like the updated error messaging. :)

eliperelman · 2017-03-16T14:10:30Z

packages/neutrino/src/neutrino.js

@@ -6,6 +6,7 @@ const Config = require('webpack-chain');
 const ora = require('ora');
 const merge = require('deepmerge');

+const cwd = process.cwd();


I'd like to not use process.cwd() here, if possible, outside of the constructor. See line 16, which introduces options.root.

eliperelman · 2017-03-16T14:14:41Z

packages/neutrino/src/neutrino.js

+  }
+
+  requirePreset(preset) {
+    const paths = [ join(cwd, preset), join(cwd, 'node_modules', preset), preset ];


If we use options.root, which is usually process.cwd(), we shouldn't need cwd. Remove inner spaces from the array brackets. Paths is intermediary, so we can remove the variable:

const module = [ join(this.options.root, preset), join(this.options.root, 'node_modules', preset), preset ].find(path => this.requirePath(path));

eliperelman · 2017-03-16T14:33:45Z

packages/neutrino/bin/neutrino

-    throw new Error(`Neutrino cannot find a module with the name or path '${preset}'. ` +
-      `Ensure this module can be found relative to the root of the project.`);
-  });
+  // Grab all presets and merge them into a single webpack-chain config instance


I just had a thought. We are using two different nomenclatures for items we are requiring: "presets" and "middleware". Presets are a carryover from pre-v5, and presets are technically middleware. Instead of naming things by what we are loading, what if we named them by the action we are going to take on them?

use

We already do this in the API. We use presets. We use middleware. The CLI interface should be synonymous with the API action.

So I'm proposing to change the CLI flag from --presets to --use. Would you make that change along with this? Then the package.json object would also change from neutrino.presets : [] to neutrino.use : [].

That would inform the name of this methods better too:

api .requireMiddleware(middleware) .forEach(middleware => api.use(middleware))

Or maybe requireMiddleware is too verbose, and the only thing we ever require is middleware, so it can be simply api.require or api.import. One thing I like about the connotation of import is that it presupposes use:

middleware.forEach(middleware => api.import(middleware)) // Does a require AND a use

TLDR; Let's change the CLI to use --use, change neutrino.presets to neutrino.use for package.json, use the variable of middleware instead of presets, and create Neutrino#import which does a require and a use.

eliperelman · 2017-03-16T14:35:06Z

Really happy with the changes this PR brings, and I've given some comments about how we can improve the API and its clarity with the CLI even more.

jaridmargolin · 2017-03-16T17:31:00Z

I will add tests for import functionality now that it is moved into the api and the functionality can more easily be run in isolation.

jaridmargolin · 2017-03-16T17:31:54Z

Other than that, I believe I addressed all concerns mentioned in the review.

eliperelman · 2017-03-16T17:43:54Z

packages/neutrino/bin/neutrino

-  .option('presets', {
-    description: 'A list of Neutrino presets used to configure the build',
+  .option('use', {
+    description: 'A list of Neutrino middlewares used to configure the build',


I know it sounds silly, but "middleware" is plural and singular. Let's use the word "middleware" over "middlewares" everywhere.

eliperelman · 2017-03-16T17:44:29Z

packages/neutrino/src/neutrino.js

+    middleware(this, options);
+  }
+
+  import (middlewares) {


Remove extra space after word import.

eliperelman · 2017-03-16T19:13:22Z

packages/neutrino/src/neutrino.js

+
+  import (middlewares) {
+    middlewares = Array.isArray(middlewares) ? middlewares : [middlewares];
+    this.requireMiddlewares(middlewares).forEach(middleware => api.use(preset));


Can probably inline the middleware (also still a reference to preset here):

this .requireMiddleware(Array.isArray(middleware) ? middleware : [middleware]) .forEach(middleware => api.use(middleware));

This is why I need to get some tests in place :)

eliperelman · 2017-03-16T19:14:13Z

packages/neutrino/src/neutrino.js

+    this.requireMiddlewares(middlewares).forEach(middleware => api.use(preset));
+  }
+
+  requireMiddlewares(middlewares) {


We can remove this method, if we do the mapping in requireMiddleware, and normalize the array. See my comment there.

eliperelman · 2017-03-16T19:16:48Z

packages/neutrino/src/neutrino.js

+    return middlewares.map(middleware => this.requireMiddleware(middleware));
+  }
+
+  requireMiddleware(middleware) {


We can make this handle singular or plural middleware, just like import:

requireMiddleware(middleware) { if (!Array.isArray(middleware)) { middleware = [middleware]; } return middleware.map(middleware => { // ... }); }

eliperelman · 2017-03-16T19:17:35Z

packages/neutrino/src/neutrino.js

+  }
+
+  requireMiddleware(middleware) {
+    const paths = [


This should be named module instead of paths.

This is why I need to get some tests in place :)

I'm actually curious how this wasn't picked up by linting in travis.

eliperelman · 2017-03-17T13:18:56Z

packages/neutrino/bin/neutrino

-        throw exception;
-      }
-    }
+  const inspect = pipe(sort, partialRight(stringify, [null, 2]), console.log, process.exit);


This function isn't dependent on anything inside of run, so let's pull this one into the outer scope.

eliperelman · 2017-03-17T13:20:17Z

packages/neutrino/bin/neutrino

 }

-run(args._[0], [...new Set(pkgPresets.concat(args.presets))]);
+run(args._[0], args).catch(err => {
+  console.error(err || `Error during ${args._[0]}`)


Missing semicolon. (I fixed the linter in another patch, these should be picked up on your next commit.)

Ahhh good. I'll rebase your changes (I've been using standard lately, and sometimes its difficult to switch between projects which don't use it).

eliperelman · 2017-03-17T13:20:47Z

packages/neutrino/src/neutrino.js

+  }
+
+  import(middleware) {
+    this.require(middleware).forEach(middleware => this.use(middleware))


eliperelman · 2017-03-17T13:20:56Z

packages/neutrino/src/neutrino.js

+  }
+
+  require(middleware) {
+    return requireMiddleware(middleware, this.options)


eliperelman · 2017-03-17T13:23:15Z

packages/neutrino/src/neutrino.js

-        return this.watcher();
-      })
-      .then(() => this.emitForAll('start', args));
+    return this.runCommand('start', args, () => {


This can be:

return this.runCommand('start', args, () => (this.getWebpackOptions.devServer ? this.devServer() : this.watcher()));

eliperelman · 2017-03-17T13:59:39Z

packages/neutrino/test/requireMiddleware_test.js

@@ -0,0 +1,45 @@
+import { join } from 'path';
+import { outputFile, remove } from 'fs-extra';


So we can use outputFile and remove below, let's make this:

import { outputFile as fsOutputFile, remove as fsRemove } from 'fs-extra';

eliperelman · 2017-03-17T13:59:50Z

packages/neutrino/test/requireMiddleware_test.js

+import requireMiddleware from '../src/requireMiddleware';
+
+const cwd = process.cwd()
+const poutputFile = pify(outputFile);


Change to outputFile.

eliperelman · 2017-03-17T13:59:58Z

packages/neutrino/test/requireMiddleware_test.js

+
+const cwd = process.cwd()
+const poutputFile = pify(outputFile);
+const premove = pify(remove);


Change to remove.

eliperelman · 2017-03-17T14:02:12Z

packages/neutrino/test/requireMiddleware_test.js

+const moduleMiddlewarePath = join(modulePath, 'index.js');
+
+
+test.before(t => {


Async functions:

test.before(async (t) => { await Promise.all([ outputFile(rootMiddlewarePath, `module.exports = 'root';`), outputFile(errorMiddlewarePath, '[;'), outputFile(moduleMiddlewarePath, `module.exports = 'mymodule';`) ]); process.chdir(rootPath); });

eliperelman · 2017-03-17T14:56:07Z

packages/neutrino/test/requireMiddleware_test.js

+  ]).then(() => process.chdir(rootPath));
+});
+
+test.after.always(t => {


test.after.always(async (t) => { await remove(rootPath); process.chdir(cwd); };

… --env option to CLI).

eliperelman

🎉

eliperelman · 2017-03-17T23:50:42Z

Very well done.

eliperelman suggested changes Mar 16, 2017

View reviewed changes

jaridmargolin force-pushed the neutrino-refactor branch 2 times, most recently from 7070398 to 08f7fd1 Compare March 16, 2017 17:21

eliperelman suggested changes Mar 16, 2017

View reviewed changes

jaridmargolin force-pushed the neutrino-refactor branch 3 times, most recently from 45f7b07 to ddbfc25 Compare March 17, 2017 01:26

eliperelman suggested changes Mar 17, 2017

View reviewed changes

jaridmargolin added 6 commits March 17, 2017 17:22

Move require/import middleware functionality to api.

6fdd8d6

Make run method in bin/neutrino self contained.

60880d8

Move run process logic to location of call.

98eec4c

Remove concept of "presets" in favor of "middleware(s)/use".

4ae503a

Add runCommand helper method to Neutrino.

339d049

Set environment variable, NODE_ENV, inside of runCommand (also adds…

27b8011

… --env option to CLI).

jaridmargolin force-pushed the neutrino-refactor branch from ddbfc25 to 19ce099 Compare March 17, 2017 21:22

Add packages/bin/neutrino to lint files list.

7f518f8

jaridmargolin force-pushed the neutrino-refactor branch from 19ce099 to 7f518f8 Compare March 17, 2017 22:44

eliperelman approved these changes Mar 17, 2017

View reviewed changes

eliperelman merged commit 69360d0 into neutrinojs:master Mar 17, 2017

jaridmargolin deleted the neutrino-refactor branch March 18, 2017 07:20

eliperelman mentioned this pull request Mar 18, 2017

Remove process.exit(0) on run. #118

Closed

		@@ -0,0 +1,45 @@
		import { join } from 'path';
		import { outputFile, remove } from 'fs-extra';

		const moduleMiddlewarePath = join(modulePath, 'index.js');


		test.before(t => {

Neutrino refactor #119

Neutrino refactor #119

Conversation

jaridmargolin commented Mar 16, 2017 • edited Loading

jaridmargolin commented Mar 16, 2017

eliperelman commented Mar 16, 2017

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

Choose a reason for hiding this comment

eliperelman commented Mar 16, 2017

jaridmargolin commented Mar 16, 2017

jaridmargolin commented Mar 16, 2017

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

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

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 left a comment

Choose a reason for hiding this comment

eliperelman commented Mar 17, 2017

jaridmargolin commented Mar 16, 2017 •

edited

Loading