Improve error message for middleware .use()d incorrectly

[edmorley]

As part of #326, one of my initial (/naive) attempts at reducing the duplication looked like:

const { join } = require('path');
const htmlTemplate = require('neutrino-middleware-html-template');

const entries = ['index', 'perf', 'logviewer', 'failureviewer', 'userguide'];

module.exports = {
  options: {
    source: 'ui/',
    output: 'dist/',
  },
  use: [
    'neutrino-preset-react',
    (neutrino) => {

      entries.forEach(entry => neutrino.config
        .entry(entry)
          .add(join(neutrino.options.source, `entry-${entry}.js`))
          .end()
        .plugin(`html-${entry}`)
          .use(htmlTemplate, {
            inject: 'body',
            filename: `${entry}.html`,
            template: join(neutrino.options.source, `${entry}.html`),
            chunks: [entry, 'vendor', 'runtime'],
          })
      );

    },
  ],
};

This unsurprisingly didn't work given that neutrino-middleware-html-template returns a config object not just a webpack plugin, so resulted in:

$ ./node_modules/.bin/neutrino build --inspect > webpack-config.js
C:\Users\Ed\src\treeherder\node_modules\webpack-chain\src\Plugin.js:8
    this.init((Plugin, args = []) => new Plugin(...args));
                                     ^

TypeError: undefined is not a function
    at init (C:\Users\Ed\src\treeherder\node_modules\webpack-chain\src\Plugin.js:8:38)
    at module.exports.toConfig (C:\Users\Ed\src\treeherder\node_modules\webpack-chain\src\Plugin.js:37:12)
    at clean.Object.assign.plugins.plugins.values.map.plugin (C:\Users\Ed\src\treeherder\node_modules\webpack-chain\src\Config.js:68:59)
    at Array.map (<anonymous>)
    at module.exports.toConfig (C:\Users\Ed\src\treeherder\node_modules\webpack-chain\src\Config.js:68:38)
    at ChainAction.Future.try.chain.chain.chain [as mapper] (C:\Users\Ed\src\treeherder\node_modules\neutrino\src\api.js:180:61)
    at ChainAction.resolved (C:\Users\Ed\src\treeherder\node_modules\fluture\index.js:708:83)
    at resolved (C:\Users\Ed\src\treeherder\node_modules\fluture\index.js:170:19)
    at Timeout.escaped [as _onTimeout] (C:\Users\Ed\src\treeherder\node_modules\fluture\index.js:42:34)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)

Whilst this was a silly mistake on my part, it would be great if the exception were clearer - since at the moment it neither refers to which line of the .neutrinorc.js is broken nor does undefined is not a function really help understand what was incorrect.

This was using Neutrino 6.2.0, webpack-chain 3.3.0 and node 8.5.0.

[eliperelman]

I'm not sure any error messaging will help here. The point of middleware is to be used by Neutrino, not by the config:

neutrino.use(htmlTemplate, { /* ... */ });

[edmorley]

As the person who was momentarily confused, I can promise better error messaging definitely would have helped. Did you mean instead that it's not really possible or worth the effort? (Either of which would be very fair points)

For you (as someone who works more closely on Neutrino/webpack), the usage above probably seems like a daft thing to do, but for someone like me who hasn't really used webpack on it's own, and whose only use of it has been via Neutrino, I didn't really understand what parts of the syntax relate to Neutrino features vs existing webpack concepts (I've now done some more webpack docs reading since).

Also having the same verb (use) be for both middleware and webpack plugins contributed to the confusion.

[eliperelman]

Yeah, I can see where use would be confusing. Maybe we should change plugin.use to something else.

My statement about better error messaging I think came across incorrect. I guess I mean the context on where this messaging would live could make this vague or unhelpful. Take the following scenario:

module.exports = (neutrino) => {
  neutrino.config.plugin('alpha').use(BetaPlugin);
};

If this middleware were to not be used by Neutrino, then it would be up to the middleware to detect that:

module.exports = (neutrino) => {
  if (!neutrino || !neutrino.config) {
    throw new Error('this middleware was not used correctly');
  }

  neutrino.config.plugin('alpha').use(BetaPlugin);
};

Is it reasonable to make all middleware enforce these checks? If not, and you want this to live in Neutrino, how then do we enforce better error messages for middleware that we aren't necessarily even touching?

[edmorley]

I agree we wouldn't want middleware to have to enforce that. I was more wondering if there was a way to do so in Neutrino, given neutrino/src/api.js is on the stack?

[eliperelman]

The only thing I can think of is doing some more with uncaught exceptions, and trying to infer problems based on exception messages.

[edmorley]

Looking at the stack for this instance, I see a few places where we could try catching exceptions and adding some more context to the output. For example in the toConfig() here: https://github.com/mozilla-neutrino/webpack-chain/blob/a669fe03ef19d40598a0a6fc203b186a39407b58/src/Plugin.js#L34-L38

Perhaps a generic toConfig() on the underlying ChainedMap (so we can re-use this for more than just plugins) that caught exceptions, dumped the name of the class on which the toConfig() was called and then re-threw, might be enough?

[eliperelman]

Yeah, I'd be willing to entertain the idea.

[edmorley]

Let's not worry about this for now.

(Were we to want this in the future, I believe a quick type check in webpack-chain's Plugin.js::use() would be sufficient.)