Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Morph Neutrino API and CLI into middleware injectors for external CLI tools #852

Merged
merged 7 commits into from May 23, 2018
Merged

Morph Neutrino API and CLI into middleware injectors for external CLI tools #852

merged 7 commits into from May 23, 2018

Conversation

eliperelman
Copy link
Member

@eliperelman eliperelman commented May 7, 2018

Edited to reflect current state of PR.

Fixes #708.
Fixes #736.
Fixes #870.
Fixes #842.
Closes #773.
Closes #839.
Closes #849.

Outstanding issues to file:

  • Update docs to reflect these API changes (#896)
  • Determine need for config-loading bins instead of forcing config file boilerplate (still unsure if we should do this because of IDE integration)
  • Modify stats to produce friendlier build output (#897)

This patch is a work-in-progress, and is quite the overhaul of Neutrino into something different. Right now there is no change to the documentation, which I will change if we decide to move forward with this.

This patch morphs Neutrino into a "preset engine" for transforming managed webpack configuration into a format that can be consumed by other CLI tools. No longer would we wrap external CLI tools like webpack, eslint, jest, karma, mocha, stylelint, etc. Instead, Neutrino could augment these tools by exposing methods for returning usable configuration.

This approach gives the user control of their CLI tool back, and they can programmatically invoke Neutrino to load their configuration, and override however they like:

webpack --mode production
// webpack.config.js
const neutrino = require('neutrino');

// Load with no config overrides:
module.exports = neutrino().webpack();

// Load with config overrides:
module.exports = neutrino().webpack(config => {
  // return whatever config you want
});

// Load with no config overrides using low-level API:
module.exports = neutrino().output('webpack');

// Load with config overrides using low-level API:
module.exports = neutrino().output('webpack', config => {
  // return whatever config you want
});

The same works for ESLint and friends too (except for mocha, since it has no config file we can be required from):

eslint src
// .eslintrc.js
const neutrino = require('neutrino');

// Load with no config overrides:
module.exports = neutrino().eslintrc();

// Load with config overrides:
module.exports = neutrino().eslintrc(config => {
  // return whatever config you want, like manipulating rules, etc.
});

  • This approach is predicated on having middleware defined in .neutrinorc.js, which shouldn't change at all.
  • This does do away with a bunch of CLI functionality like --inspect or --use, but those become less useful if we are just an interface to external CLI tools. We can discuss potentially bringing those back. This is now back in the form of the CLI with neutrino --inspect.
  • All environment variables are gone. We rely only on webpack's mode value. We internally only use webpack's mode value, but still set process.env.NODE_ENV if not set based on the mode, or defaulted to production.
  • There is no longer a create-project tool, since we cannot force users to choose between competing tools like webpack-cli or webpack-command, and webpack-dev-server or webpack-serve. The create-project tool is back, and we opt for webpack-cli and webpack-dev-server as opinions for now.

I'm sure there are other major things I am neglecting to mention. Let's discuss! I see so many benefits to this, including reduced maintenance burden and the ability to allow people to use Neutrino without dumping their existing tools.

cc: @mozilla-neutrino/core-contributors

@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented May 7, 2018

Ah, one thing I forgot to mention is that you can override configuration still using neutrinorc with the webpack-chain instance. This means you can now override using whichever method you prefer:

// Use neutrinorc for deterministic configurability with
// the chainable API
module.exports = {
  use: [
    '@neutrinojs/react',
    neutrino => {
      neutrino.config.devtool('source-map');
    }
  ]
};
// Override from the configuration file of your CLI tool.
// This requires that you switch away from using the internal
// Neutrino CLI loader to making the tool use its own config file
// again, e.g.
// from:
// neutrino webpack --mode production
// to:
// webpack --mode production

// webpack.config.js
const neutrino = require('neutrino');

module.exports = neutrino().webpack(config => ({
  ...config,
  devtool: 'source-map'
});

@timkelty
Copy link
Contributor

@timkelty timkelty commented May 7, 2018

Hope to review this week, but I'm a big fan of moving in this direction!

@SeeThruHead
Copy link
Contributor

@SeeThruHead SeeThruHead commented May 7, 2018

Absolutely love this.

@edmorley
Copy link
Member

@edmorley edmorley commented May 7, 2018

Many thanks for working on this - as you know I'm keen to try and make it easier to use native tools with Neutrino / reduce the cost of maintenance on our side / possibly reduce the overhead of Neutrino in startup time :-)

I tested this by comparing master (though I refreshed the lockfile on master only, so webpack 4.8.0 on both) and this PR via the following steps:

  1. From monorepo root: rm -rf node_modules packages/*/node_modules && yarn && yarn link:all
  2. cd .. && create-project test-<name>
  3. Give the following answers: Web -> React -> Jest -> AirBnb
  4. cd test-<name>
  5. Edit the generated package.json to remove neutrino and @neutrinojs/*
  6. rm -rf node_modules yarn.lock && yarn
  7. yarn link neutrino @neutrinojs/react @neutrinojs/jest @neutrinojs/airbnb
  8. Run yarn build 5 times and take the quickest invocation according to time

On master I get:

$ time yarn build
yarn run v1.6.0
$ neutrino build
√ Building project completed
Hash: 644638fa61693d1f28fe
Version: webpack 4.8.0
Time: 3422ms
Built at: 2018-05-07 21:16:03
                          Asset       Size  Chunks             Chunk Names
runtime.a5d5684f37863467cd6d.js   1.04 KiB       0  [emitted]  runtime
      1.f5d8aa6c2a2a15538a95.js    106 KiB       1  [emitted]
 index.96536b326bb3b272ec66.css   18 bytes       2  [emitted]  index
  index.0705adefdc1c7579150f.js  562 bytes       2  [emitted]  index
                     index.html  565 bytes          [emitted]
Entrypoint index = runtime.a5d5684f37863467cd6d.js 1.f5d8aa6c2a2a15538a95.js index.96536b326bb3b272ec66.css index.0705adefdc1c7579150f.js
Done in 7.73s.

real    0m8.294s
user    0m0.000s
sys     0m0.015s

Trying with this PR I got:

$ yarn build
yarn run v1.6.0
$ neutrino webpack --mode production
events.js:165
      throw er; // Unhandled 'error' event
      ^

Error: spawn webpack ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:201:19)
    at onErrorNT (internal/child_process.js:379:16)

I presume this is due to needing child_process.spawn() changes for compatibility with Windows. I tried adding shell: true, which got further, but still failed with Error: Cannot find module '.neutrinorc.js', so I skipped this approach for now.

I instead created a webpack.config.js like so:

const neutrino = require('neutrino');
module.exports = neutrino().webpack();

And then tried using webpack directly:

$ time yarn webpack --mode production
yarn run v1.6.0
$ C:\Users\Ed\src\test-react-new\node_modules\.bin\webpack --mode production
Hash: 6d910272a91fdf310949
Version: webpack 4.8.0
Time: 2891ms
Built at: 2018-05-07 22:01:05
                          Asset       Size  Chunks             Chunk Names
runtime.d3f7781965753ba20f1b.js   1.04 KiB       0  [emitted]  runtime
      1.083d16c533c13131a3f5.js    106 KiB       1  [emitted]
                      index.css   18 bytes       2  [emitted]  index
  index.e329700ad1ece9219d57.js   1.04 KiB       2  [emitted]  index
                     index.html  548 bytes          [emitted]
Entrypoint index = runtime.d3f7781965753ba20f1b.js 1.083d16c533c13131a3f5.js index.css index.e329700ad1ece9219d57.js
 [6] ./src/App.css 39 bytes {2} [built]
 [7] ./src/App.jsx 1.01 KiB {2} [built]
[19] ./src/index.jsx 635 bytes {2} [built]
[20] multi ./src/index 28 bytes {2} [built]
    + 17 hidden modules
Child html-webpack-plugin for "index.html":
     1 asset
    Entrypoint undefined = index.html
    [0] (webpack)/buildin/module.js 497 bytes {0} [built]
    [1] (webpack)/buildin/global.js 489 bytes {0} [built]
        + 2 hidden modules
Child mini-css-extract-plugin ../neutrino-dev/node_modules/css-loader/index.js??ref--7-1!src/App.css:
    Entrypoint mini-css-extract-plugin = *
    [1] ../neutrino-dev/node_modules/css-loader??ref--7-1!./src/App.css 204 bytes {0} [built]
        + 1 hidden module
Done in 6.14s.

real    0m6.677s
user    0m0.000s
sys     0m0.015s

It looks like the configuration differs slightly:

  • index.css no longer has a hashed filename
  • index.js is larger
  • the stats output has changed

...however the build time reduced by 1.5s, which is promising (though might just be the different build options?).

Some other things I spotted:

  • create-project's initial lint didn't complete (the yarn webpack --mode production is after I fixed the issues)
  • I had problems getting the create-project's generated .eslintrc.js to work - running eslint failed with:
$ yarn eslint --ext=js,jsx,vue,ts,tsx,mjs src --fix
yarn run v1.6.0
$ C:\Users\Ed\src\test-react-new\node_modules\.bin\eslint --ext=js,jsx,vue,ts,tsx,mjs src --fix
Cannot read config file: C:\Users\Ed\src\test-react-new\.eslintrc.js
Error: Cannot find module '.neutrinorc'
Error: Cannot read config file: C:\Users\Ed\src\test-react-new\.eslintrc.js
Error: Cannot find module '.neutrinorc'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:548:15)
  • when we get to the point of adding more polish to this approach, it would be good to improve the error message for people whose yarn build command still tries to run neutrino build, since it currently fails with the confusing:
$ yarn build
yarn run v1.6.0
$ neutrino build
C:\Users\Ed\src\neutrino-dev\packages\neutrino\bin\neutrino.js:16
spawn(command, [...args, ...commands[command]], {
                                    ^

TypeError: commands[command] is not iterable
    at Object.<anonymous> (C:\Users\Ed\src\neutrino-dev\packages\neutrino\bin\neutrino.js:16:37)

@edmorley
Copy link
Member

@edmorley edmorley commented May 7, 2018

This is similar to how nyc works, by calling a Neutrino CLI which just loads middleware and then spawns the given command passing in a config file.

Could this part be avoided by instead defining the package.json build script as:
webpack --mode production --config node_modules/neutrino/src/foo.js

I know passing a node_modules path isn't pretty, but given neutrino is a top-level dependency, we don't need to sorry about hoisting, right? I'm just wondering if we can get away with keeping the number of API usage permutations to a minimum, to reduce complexity/maintenance/potential for bugs.

Also, what are your thoughts on starting to use .babelrc.js with this approach too? (Or the new babel.config.js which avoids some of .babelrc*'s issues; see babel/babel#7358.) It would seem that many tools "just work" when a babelrc is preset (eg babel-jest) - so it might simplify several other presets.

@edmorley
Copy link
Member

@edmorley edmorley commented May 8, 2018

It looks like the configuration differs slightly:

  • index.css no longer has a hashed filename
  • index.js is larger

Ah the former is because packages/style-loader/index.js uses neutrino.config.mode === 'production' rather than neutrino.config.get('mode') === 'production'.

The latter is because the Jest preset's Babel settings are being applied even for .webpack(), and plugin-transform-modules-commonjs increases the output size.

@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented May 8, 2018

Could this part be avoided by instead defining the package.json build script as:
webpack --mode production --config node_modules/neutrino/src/foo.js

Yes, that is certainly possible. If we are fine with a little loss in ergonomics here, we can move forward with it.

Also, what are your thoughts on starting to use .babelrc.js with this approach too?

This should be totally possible now, just need to add a babel() method.

Ah the former is because packages/style-loader/index.js uses neutrino.config.mode === 'production' rather than neutrino.config.get('mode') === 'production'.

Good catch, yay bugs!

The latter is because the Jest preset's Babel settings are being applied even for .webpack(), and plugin-transform-modules-commonjs increases the output size.

🤔 Hmm yeah, let me think about this one.

@eliperelman eliperelman requested a review from edmorley May 11, 2018
@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented May 11, 2018

With my current changeset, the only way to affect configuration is via .neutrinorc.js or an override in a respective config file, e.g. webpack.config.js or .eslintrc.js. I took away the ability to do webpack node_modules/neutrino/whatever for now, so you need a webpack.config.js with:

const neutrino = require('neutrino');

module.exports = neutrino().webpack();

I think we should discuss if this needs to be a requirement, or if we should provide the scripts to pass to these external bins.

Copy link
Member

@edmorley edmorley left a comment

I tested this locally with a webpack-cli build using --mode production, which worked great. It initially only tooks 15s on a warm rebuild which I tracked down to my .neutrinorc.js having a NODE_ENV === 'production' conditional for setting devtool source-map.

I think we still need to set process.env.NODE_ENV according to mode and/or whether command={jest,...} etc. I also can't decide whether our conditionals throughout should be from mode, command or NODE_ENV.

I think we might still need some inspect functionality (eg looking for argv.inspect), but perhaps our options will be clearer once that feature is upstreamed to webpack-chain.

I ran out of time to test further (need to head out for a show) - but will take another look tomorrow at dev server/eslint/....

Oh and sorry for so many "separate PR" comments -- I wouldn't bother so much but it's already +2400, -5700 hehe.

package.json Outdated
"prettier": "^1.12.1"
"prettier": "^1.12.1",
"stylelint": "^8.4.0",
"webpack": "^4.7.0",
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

latest webpack is 4.8.2 (not that it matters much given tilde..)

@@ -4,7 +4,7 @@ module.exports = (neutrino, options = {}) => {
neutrino.config
.plugin(options.pluginId || 'banner')
.use(BannerPlugin, [{
banner: 'require(\'source-map-support\').install();',
banner: `require('source-map-support').install();`,
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split this change to a separate PR?

delete options.pluginId;

Reflect.deleteProperty(options, 'paths');
Reflect.deleteProperty(options, 'pluginId');
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR? :-)

@@ -1,7 +1,8 @@
const babelMerge = require('babel-merge');

module.exports = (neutrino, options = {}) => neutrino.config.module
.rule(options.ruleId || 'compile')
module.exports = (neutrino, options = {}) => {
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR? :-)

module.exports = ({ config }, envs = []) => config
.plugin('env')
.use(EnvironmentPlugin, envs);
module.exports = ({ config }, envs = []) => {
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR? :-)

@@ -10,4 +10,4 @@ module.exports = ({ config }, opts = {}) => {
config
.plugin(options.pluginId)
.use(OptimizeCssPlugin, [options.plugin]);
}
};
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate PR? :-)

@@ -252,13 +248,4 @@ module.exports = (neutrino, opts = {}) => {

config.output.filename('[name].[chunkhash].js');
});

neutrino.on('prerun', () => {
if (neutrino.config.entryPoints.has('vendor')) {
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good if we could add this check somewhere else - since it's a likely footgun for people upgrading that results in larger/slower builds (vs the other cases where it's just about cleaning up removed preferences).

.eslintignore Outdated
@@ -1,4 +1,6 @@
packages/**/node_modules/
# Remove ignore of neutrinorc since ESLint ignores dotfiles by default
!.neutrinorc.js
# TODO: Fix lint errors in our tests and remove this line
packages/**/test/
# The templates have any lint errors fixed during project creation,

This comment was marked as outdated.

"eslint-config-airbnb-base": "^12.1.0",
"eslint-plugin-import": "^2.11.0"
},
"peerDependencies": {
"eslint": "^4.0.0",
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @neutrinojs/eslint package will already have a peerDependency on eslint, which will be inherited. Though might be clearer for users inspecting only the top-level package.json to be explicit? I can't decide. (The same also applies to eg the webpack peerDep in @neutrinojs/font-loader since file-loader has it's own peerDep etc)

Copy link
Member Author

@eliperelman eliperelman May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I added this since I figured it would be easier to ensure users would be notified from their top-level peer dependencies, but I am fine with removing some of them if they are bloating.

Copy link
Member

@edmorley edmorley May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think given we're now needing users to add a bunch of peerDeps perhaps we need to have it as clear as possible to them :-)

destination &&
destination.eslint &&
destination.eslint.rules
) || {};
const rules = deepmerge(sourceRules, destinationRules, {
arrayMerge(source, destination) {
return destination;
Copy link
Member

@edmorley edmorley May 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't select the line, since it's further down below - but there's a leftover neutrino.options.command !== 'start'.

I wonder if command could be renamed to something else so third-party middleware / end-users' .neutrinorc.js conditionals fail hard rather than silently not matching?

Copy link
Member Author

@eliperelman eliperelman May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think target was suggested at one point, e.g. neutrino.options.target, but I'm not convinced it wouldn't be confused with config.target.

@edmorley
Copy link
Member

@edmorley edmorley commented May 12, 2018

I think we might still need some inspect functionality (eg looking for argv.inspect)

Oh and the debug functionality (for me, main use I have had for it is preset-env's debug output).

@edmorley
Copy link
Member

@edmorley edmorley commented May 14, 2018

I wonder if we should also make webpack() and friends throw if mode is not set? (ie if the user forgets to --mode ...)

@ringods
Copy link
Contributor

@ringods ringods commented May 17, 2018

@eliperelman testing my multi-compiler question from #876 against this rework.

With the new setup, would I have something like this in my webpack.config.js?

const neutrino = require('neutrino');

module.exports = neutrino().use('my-preset-generating-multi-compiler')

What would the api be then for a preset to return a list of webpack configurations?
I'm not understanding it completely at the moment.

neutrino.config.when(
neutrino.config.module.rules.has('compile') &&
neutrino.options.command === 'jest',
() => {
Copy link
Member

@edmorley edmorley May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the false case isn't used, and we're not using a passed (config), this might be cleaner as:

  if (
    neutrino.config.module.rules.has("compile") &&
    neutrino.options.command === "jest"
  ) {
    // ...
  }

@@ -2,7 +2,7 @@
"name": "@neutrinojs/mocha",
"version": "8.2.0",
"description": "Neutrino preset for testing Neutrino projects with Mocha",
"main": "src/index.js",
Copy link
Member

@edmorley edmorley May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the jest preset case, could the file be left under src/ for now, so that git knows it's existing content and displays the actual diff? (Happy for the file to be moved either in a PR before this lands, or sometime after)

.command('karma')
.init();

if (neutrino.config.module.rules.has('compile')) {
Copy link
Member

@edmorley edmorley May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like these really belong in the preset rather than in the Neutrino package. Could we instead maybe do something like either:

  • make these files be mostly a shell that require()s the relevant parts from the preset
  • have the presets dynamically register new commands, so we don't need to hardcode each test runner in the neutrino package?

Copy link
Member Author

@eliperelman eliperelman May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I hadn't thought of it this way before, and it would take re-working the adapter pattern, but let me see what I can do.

@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented May 17, 2018

@ringods I think the idea would be that you could export multiple Neutrino calls:

EDITED

// webpack.config.js
const neutrino = require('neutrino');

module.exports = [
  neutrino().output('webpack'),
  neutrino(middleware).output('webpack')
];

@edmorley
Copy link
Member

@edmorley edmorley commented May 17, 2018

To add to the commit message:

Fixes #708.
Fixes #736.
Fixes #870.
Closes #773.
Closes #849.

We should also check whether it fixes any of:
#842 (whilst Neutrino test won't exist, we should check for say .mocha() when no mocha preset)
#839
#382 (though I suspect this one will need extra changes; notably shifting to making eslint-loader use eslintrc as the source of truth)

@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented May 17, 2018

Updated description to reflect removal of adapter pattern, and now using:

neutrino().output(name, override);

neutrino().output('webpack');
neutrino().output('webpack', config => {
  return config;
});


const err = t.throws(() => api.use(require('..'), { minify: { image: true } }));
t.true(err.message.includes('The minify.image option has been removed'));
});

test('throws when vendor entrypoint defined', async t => {
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you re-add this test? (though it will need adjusting)

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this test to inside neutrino since this wasn't web's concern.

@@ -92,7 +93,6 @@ module.exports = (neutrino, opts = {}) => {
],
presets: [
[require.resolve('@babel/preset-env'), {
debug: neutrino.options.debug,
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you re-add this?

@@ -31,7 +31,6 @@
"@neutrinojs/loader-merge": "^8.2.0",
"@neutrinojs/web": "^8.2.0",
"deepmerge": "^1.5.2",
"eslint": "^4.19.1",
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess now if people don't add eslint to their package.json they will get peer dependency warnings from eslint-plugin-react (and similar for the vue/preact presets etc). However maybe that's ok and their choice to save the dependency?

The only option I can think of (other than leaving this dependency in here), is to stop the react preset from including the eslint react plugin, and then airbnb guide users can just use the airbnb preset (which does include the plugin), and for standardjs perhaps we'd have to add a second "standardjs-react" preset? (Maybe not worth it...)

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, hadn't thought of this one. I think the easiest thing at the moment would be to leave it until we think of something better, if possible.

@@ -0,0 +1,51 @@
import test from 'ava';
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome for the extra tests!


t.notDeepEqual(api.config.toConfig(), {});
});

test('throws when trying to call() a non-registered command', t => {
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should turn this into a test checking for the Unable to find an output handler named behaviour?

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this to the new package_test.

@@ -2,16 +2,11 @@
"name": "neutrino",
"version": "8.2.0",
"description": "Create and build JS applications with managed configurations",
"main": "src/index.js",
"bin": {
"neutrino": "./bin/neutrino.js"
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should leave a bin script that outputs something like "The neutrino command has been removed. See migration guide"?

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

"karma-coverage": "^1.1.2",
"karma-mocha": "^1.3.0",
"karma-mocha-reporter": "^2.2.5",
"karma-webpack": "^3.0.0",
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of these are still needed?

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved them to peer deps, although I'm not entirely happy about that.

});

test('exposes eslintrc config', t => {
t.is(typeof Neutrino().use(mw()).call('eslintrc'), 'object');
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we convert this and the other "expose XYZ" to checking that ...output('...') works?

package.json Outdated
@@ -34,17 +33,21 @@
},
"devDependencies": {
"auto-changelog": "^1.4.6",
"ava": "^0.25.0",
"ava": "1.0.0-beta.4",
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sneaking in more changes again hehe.. :-)

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase artifact. I'll fix. 😄

@@ -22,5 +23,6 @@ const name = basename(directory);
env.run('create-project', {
directory,
name,
registry: cli.registry,
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change so that the global yarn registry setting doesn't need to be adjusted? (And so can be removed from the script.) If so, this would be good since means running these tests don't affect global state locally.

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, yes. You can either pass --registry during create-project, or you can specify the registry in the test generator (see the project() function in test/cli_test).

Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like this is unrelated to the other changes here, and so belongs in a separate PR?
I'll open one for some of these cleanup things now.

@eliperelman
Copy link
Member Author

@eliperelman eliperelman commented May 18, 2018

OK, I think have the latest round of review comments addressed.

Copy link
Member

@edmorley edmorley left a comment

Posting this so you don't have to wait for me to finish up the rest of the testing etc first.

It occurred to me for could always replace the .output('name') API with .webpack() et al, by making use of ES6 Proxy traps to catch unknown commands and handle them gracefully? (Which I presume is the reason why the switch to .output()?). Either way works for me though, so happy to go with your choice :-)

@@ -25,8 +25,7 @@
"dependencies": {
"@babel/core": "^7.0.0-beta.46",
"babel-loader": "^8.0.0-beta.2",
"babel-merge": "^1.1.1",
"webpack": "^4.7.0"
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move this to peerDependencies rather than removing it?
eg @neutrinojs/clean kept is as a peer dep even though index.js doesn't refer to it.

Perhaps it might just be easier to add the webpack peer dep to every single Neutrino package to save having to think about it? (And to make it 200% clear to people using Neutrino what version of webpack we support.) All presets have to be used with Neutrino, which already has a peer dep on webpack, so won't really affect whether people see warnings or not. (Though the jest preset doesn't technically require it I suppose..)

Copy link
Member Author

@eliperelman eliperelman May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some people have voiced their want to use Neutrino middleware to test or lint things without actually building, so I would want to only provide it where necessary, if possible.

I think the reason clean keeps it is because the webpack plugin also peerDeps on webpack.

Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case babel-loader has a peer dep on webpack, so if we're going to explicitly mirror the deps peer deps, we should add it to compile-loader too.

@@ -22,5 +23,6 @@ const name = basename(directory);
env.run('create-project', {
directory,
name,
registry: cli.registry,
Copy link
Member

@edmorley edmorley May 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like this is unrelated to the other changes here, and so belongs in a separate PR?
I'll open one for some of these cleanup things now.

eliperelman added 2 commits May 18, 2018
… tools

Remove all CLIs, remove create project, expose command name from each config method

Fix test references to API

Simplify adapter pattern for overriding tool options

More webpack peerDeps changes

Remove irrelevant changes from patch

Reset jest source files location, upgrade to latest webpack

Fail lint if mode is production

Add support for options.debug and options.inspect

Throw if vendor entrypoint exists

Re-work create-project to support external CLIs and test with verdaccio

Fix bugs in beta verdaccio setup

Remove localhost yarn.lock entries :/

Trying single-instance tests again in CI

Removing unused environment variable detection in tests

Add back karma and mocha middleware for eslint settings

Experiment with using ramdisk for yarn cache

Use updated yarn cache directory

Update yarn.lock after rebase

Remove adapter pattern, move output handlers to middleware, parse --mode from argv

Fix output command for create-project mocha

Removing rebase artifacts

Use babel register from mocha output options

Remove reference to old yarn cache script

Add tests for exposing commands, add peer deps for karma

Revert back to previous ava

Use a proxy to expose output handlers as named methods on neutrino package

Revert changes to eslintignore and create-project that were out of scope
Copy link
Member

@edmorley edmorley left a comment

Last few things and hopefully this should be good to go! :-)

@@ -25,8 +25,7 @@
"dependencies": {
"@babel/core": "^7.0.0-beta.46",
"babel-loader": "^8.0.0-beta.2",
"babel-merge": "^1.1.1",
"webpack": "^4.7.0"
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case babel-loader has a peer dep on webpack, so if we're going to explicitly mirror the deps peer deps, we should add it to compile-loader too.

},
"peerDependencies": {
"eslint": "^4.0.0",
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint-loader has a peer dep on webpack, so webpack needs adding here, if we're following that pattern. Similarly, should we then add it to @neutrinojs/airbnb and @neutrinojs/airbnb-base to, since they depend on @neutrinojs/eslint?

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess so.

},
"peerDependencies": {
"jest": "^22.0.0",

This comment was marked as outdated.

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this one feels kinda wrong since the addition of the linting is optional based on existing lint rule. Thoughts on leaving this one out?

Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah sorry we do the same for the react preset etc, so ignore this

@@ -22,18 +22,16 @@
"yarn": ">=1.2.1"
},
"dependencies": {
"@babel/core": "^7.0.0-beta.46",
"@babel/core": "^7.0.0-beta.47",
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version and the one in the mocha preset is now out of sync with the others in the repo. Could you revert the changes to the @babel/* deps? (At some point in the future I'm sure we'll hand bump again, but this is in-range and it was only done recently in #848.)

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing, this was from moving the .jest() command from neutrino package to jest.

@@ -1,130 +1,9 @@
#!/usr/bin/env node
#! /usr/bin/env node
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray whitespace here (shebangs normally don't have additional whitespace before the interpreter; not all POSIX implementations support it)

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

"neutrino": "^8.0.0"
"karma": "^2.0.0",
"karma-cli": "^1.0.0",
"mocha": "^5.0.0",
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm presuming mocha can't be moved to dependencies due to something like the karma frameworks pref not accepting absolute depdency paths?

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was mostly to enable usage of karma and mocha presets in the same repo.

"@neutrinojs/loader-merge": "^8.2.0",
"change-case": "^3.0.2",
"mocha": "^5.1.1",
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this preset, should mocha now be listed in peerDependencies, rather than removed?

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sigh, yes.

scripts.start = `${packages.NEUTRINO} start`;
scripts.start = this.data.project === '@neutrinojs/node'
? 'webpack --watch --mode development'
: 'webpack-serve --mode development';
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running yarn start on a create-project with options React+Jest+AirBnb, I get:

$ yarn start
yarn run v1.6.0
(node:6040) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
$ webpack-serve --mode development
× 「config」: An error ocurred while trying to find a config file
              Did you forget to specify a --require?
C:\Users\Ed\src\test-project\node_modules\@webpack-contrib\config-loader\lib\load.js:84
    throw new LoadConfigError(e, configPath);
    ^

LoadConfigError: Cannot find module 'webpack-dev-server/client'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:571:15)
    at Function.require.resolve (C:\Users\Ed\src\test-project\node_modules\webpack-serve\node_modules\v8-compile-cache\v8-compile-cache.js:162:23)
    at config.entry.batch.entry (C:\Users\Ed\src\test-project\node_modules\@neutrinojs\web\index.js:235:43)

I guess we have no coverage of this at the moment?

Perhaps in test_cli.js we can do either (or both) of:

  • test webpack build using --mode development
  • see if we can spawn the dev server via yarn start, then if it starts successfully, kill it after a few seconds

Until then perhaps we need to hand-test (using create-project and --registry) all of the top-level preset's yarn start commands?

Also, I think the reason the unit tests didn't catch this is because the repo root package.json contains webpack-dev-server (when I think it perhaps shouldn't?).

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooooooh, this is a problem. I don't think we can use webpack-serve out-of-the-box because of our hardcoded entries in web for dev server:

https://github.com/mozilla-neutrino/neutrino-dev/blob/master/packages/web/index.js#L235

Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should switch back to webpack-dev-server for now (also reduces the amount changed in one PR) and we can figure it out later?

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You read my mind.

Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps in test_cli.js we can do either (or both) of:

  • test webpack build using --mode development
  • see if we can spawn the dev server via yarn start, then if it starts successfully, kill it after a few seconds

I've filed #886 for this.


// devServer :: (Object config -> Object api) -> Future () Function
const devServer = (config, { options }) => Future
.of(merge({ devServer: { host: 'localhost', port: 5000, noInfo: !options.debug } }, config))
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the noInfo: !options.debug need to move to packages/dev-server/index.js now? Or are we leaving it to the user to pass the options to webpack-serve?

The latter seems to make sense, I was just wondering if with the current settings things are going to get verbose. I think after this PR we'll probably want to revisit our customisation of stats entirely, so perhaps this can wait until then?

Copy link
Member Author

@eliperelman eliperelman May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's follow up with stats changes.

.option('debug', {
description: 'Run in debug mode',
boolean: true,
default: false,
Copy link
Member

@edmorley edmorley May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this PR neutrino.options.debug now defaults to undefined rather than false, which means clean-webpack-plugin now gives verbose output (its default) when it didn't before. I think we need to make it default to false again.

On that note, process.env.NODE_ENV is also undefined when it wasn't before. I think we need to set that explicitly again too, per:
#852 (review)

@edmorley
Copy link
Member

@edmorley edmorley commented May 23, 2018

The Netlify build on master is failing with:
File not found: /opt/build/repo/docs/packages/fork.md
(https://app.netlify.com/sites/neutrinojs/deploys/5b05c3691f12b73cb85e935d)

Could you update mkdocs.yml for the fork preset removal?

edmorley added a commit that referenced this issue May 24, 2018
The `fork` preset was removed by #852, but it still referenced by
the newly added mkdocs configuration.
@edmorley edmorley mentioned this