@neutrinojs/library externals cause Karma "Uncaught ReferenceError: require is not defined"

[helfi92]

Bug

• What version of Neutrino are you using?

9.0.0-rc.0

• Are you trying to use any presets? If so, which ones, and what versions?

@neutrinojs/library@9.0.0-rc.0
@neutrinojs/karma@9.0.0-rc.0

• Are you using the Yarn client or the npm client? What version?

yarn v1.12.3

• What version of Node.js are you using?

10.0.0

• What operating system are you using?

macOS High Sierra

• What did you do?

Migrate a project from v8 to v9

• What did you expect to happen?

Tests still run

• What actually happened, contrary to your expectations?

karma start --single-run throws an error in the terminal

$ karma start --single-run

START:
ℹ ｢wdm｣: Time: 52ms
ℹ ｢wdm｣: Compiled successfully.
ℹ ｢wdm｣: Compiling...
ℹ ｢wdm｣: Time: 667ms
ℹ ｢wdm｣: Compiled successfully.
11 12 2018 09:46:14.553:INFO [karma-server]: Karma v3.1.3 server started at http://0.0.0.0:9876/
11 12 2018 09:46:14.555:INFO [launcher]: Launching browsers ChromeHeadless with concurrency unlimited
11 12 2018 09:46:14.562:INFO [launcher]: Starting browser ChromeHeadless
11 12 2018 09:46:14.958:INFO [HeadlessChrome 71.0.3578 (Mac OS X 10.13.4)]: Connected on socket 3u9WFcQb4PFNecstAAAA with id 98496319
HeadlessChrome 71.0.3578 (Mac OS X 10.13.4) ERROR
  {
    "message": "Uncaught ReferenceError: require is not defined\nat test/simple_test.js:930:1\n\nReferenceError: require is not defined\n    at Object.chai (test/simple_test.js:930:1)\n    at __webpack_require__ (test/simple_test.js:30:30)\n    at Module../test/simple_test.js (test/simple_test.js:801:62)\n    at __webpack_require__ (test/simple_test.js:30:30)\n    at test/simple_test.js:94:18\n    at test/simple_test.js:97:10\n    at webpackUniversalModuleDefinition (test/simple_test.js:9:26)\n    at test/simple_test.js:10:3",
    "str": "Uncaught ReferenceError: require is not defined\nat test/simple_test.js:930:1\n\nReferenceError: require is not defined\n    at Object.chai (test/simple_test.js:930:1)\n    at __webpack_require__ (test/simple_test.js:30:30)\n    at Module../test/simple_test.js (test/simple_test.js:801:62)\n    at __webpack_require__ (test/simple_test.js:30:30)\n    at test/simple_test.js:94:18\n    at test/simple_test.js:97:10\n    at webpackUniversalModuleDefinition (test/simple_test.js:9:26)\n    at test/simple_test.js:10:3"
  }

[edmorley]

Could you push the branch that's failing so I can take a look?
Super busy at the moment so a reproducible test case would really help :-)

[helfi92]

Apologies, I should have sent a test case earlier. taskcluster/taskcluster-client-web#38 :) Thanks in advance.

[edmorley]

Thank you for the testcase, I was able to reproduce locally and have tracked it down to the following:

1. As of Morph Neutrino API and CLI into middleware injectors for external CLI tools #852, @neutrinojs/library always sets neutrino.config.externals, when previously it was only set if process.env.NODE_ENV !== 'test'. This means that when karma-webpack pre-processes the source, it leaves behind the require()s for packages in dependencies (by design, see externals docs), which causes the tests to fail, since they are run in the browser without an additional webpack/rollup/... build in-between. The solution is to re-add the NODE_ENV conditional inside @neutrinojs/library (plus remove the duplicate externals call):
   https://github.com/neutrinojs/neutrino/blob/v8.3.0/packages/library/index.js#L147
   

   neutrino/packages/library/index.js

   Lines 66 to 69 in 448c82c

   	.when(options.externals, config => config.externals([nodeExternals(options.externals)]))
   	.when(hasSourceMap, () => neutrino.use(banner))
   	.devtool('source-map')
   	.externals([nodeExternals()])

Fixing (1) resolves the original error, but then leaves us with:
ReferenceError: regeneratorRuntime is not defined

...which brings us to:

2. As of Target IE9 as the browser to support ES5 compilation #994, @neutrinojs/library generates output that is compatible with IE9 (the idea being to make sure it's fully ES5 compatible). However since Babel has been told to target IE9, it wants to use regenerator-runtime (to polyfill async functions) which isn't available since @babel/polyfill isn't imported in taskcluster-client-web's entrypoint at present. This is mostly unnecessary for taskcluster-client-web's target audience given the widespread async support in modern browsers (https://caniuse.com/#feat=async-functions).

The options appear to be:
a) Add a @babel/polyfill import to taskcluster-client-web (and on Neutrino's side, document the need to do this when using @neutrinojs/library)
b) Partially revert #994 and default to last 2 browser versions like before, but add an option (or document) how to generate IE9 compatible output for those that need it.
c) Leave the default at 'ie 9', but make it easier to override the browsers list when using @neutrinojs/library, since it doesn't have a shorthand helper like @neutrinojs/web does. This would avoid having to use boilerplate like:

    ['@neutrinojs/library', {
      // ...
      babel: {
        presets: [
          [
            require.resolve('@babel/preset-env'),
            {
              debug: false,
              useBuiltIns: 'entry',
              targets: {
                browsers: [
                  'last 2 Chrome versions',
                  'last 2 Firefox versions',
                  'last 2 Edge versions',
                  'last 2 Opera versions',
                  'last 2 Safari versions',
                  'last 2 iOS versions'
                ],
              }
            }
          ]
        ],
      },
    }],

(or a .tap() equivalent that saves duplication of existing options)

--

@neutrinojs/core-contributors - thoughts about (2)?

[edmorley]

1. The solution is to re-add the NODE_ENV conditional inside `@neutrinojs/library` (plus remove the duplicate `externals` call)

Hmm actually, perhaps @neutrinojs/karma should be the one that clears out externals, since it's the one that needs it? Thoughts?

[helfi92]

Regarding

    ['@neutrinojs/library', {
      // ...
      babel: {
        presets: [
          [
            require.resolve('@babel/preset-env'),
            {
              debug: false,
              useBuiltIns: 'entry',
              targets: {
                browsers: [
                  'last 2 Chrome versions',
                  'last 2 Firefox versions',
                  'last 2 Edge versions',
                  'last 2 Opera versions',
                  'last 2 Safari versions',
                  'last 2 iOS versions'
                ],
              }
            }
          ]
        ],
      },
    }],

This would perform a deepmerge instead of overriding. There will still be a ie 9 target. I guess we have to override this property programmatically via .tap.

[helfi92]

@neutrinojs/karma should be the one that clears out externals, since it's the one that needs it?

Makes sense to me.

[edmorley]

This would perform a deepmerge instead of overriding. There will still be a ie 9 target. I guess we have to override this property programmatically.

This behaviour didn't occur when I tested locally.
Note: the merge is performed by babel-merge rather than deepmerge:

neutrino/packages/library/index.js

Line 33 in 448c82c

babel: babelMerge({

[helfi92]

With

['@neutrinojs/library', {
  // ...
  babel: {
    plugins: [
      require.resolve('@babel/plugin-proposal-class-properties'),
    ],
    presets: [
      [
        // TODO: Override using tap() so as to not have to repeat the
        // existing debug/useBuiltIns options. (Or remove entirely if
        // @neutrinojs/library stops defaulting to IE 9 target.)
        require.resolve('@babel/preset-env'),
        {
          debug: false,
          useBuiltIns: 'entry',
          targets: {
            // Override browsers from its default value of 'ie 9'
            browsers: [
              'last 2 Chrome versions',
              'last 2 Firefox versions',
              'last 2 Edge versions',
              'last 2 Opera versions',
              'last 2 Safari versions',
              'last 2 iOS versions'
            ],
          }
        }
      ]
    ],
  },
}],

neutrino --inspect --mode test shows:

use: [
          /* neutrino.config.module.rule('compile').use('babel') */
          {
            loader: '/Users/haali/Documents/Mozilla/projects/taskcluster-client-web/node_modules/@neutrinojs/compile-loader/node_modules/babel-loader/lib/index.js',
            options: {
              cacheDirectory: true,
              babelrc: false,
              configFile: false,
              presets: [
                [
                  '/Users/haali/Documents/Mozilla/projects/taskcluster-client-web/node_modules/@neutrinojs/library/node_modules/@babel/preset-env/lib/index.js',
                  {
                    debug: false,
                    useBuiltIns: 'entry',
                    targets: {
                      browsers: 'ie 9'
                    }
                  }
                ],
                [
                  '/Users/haali/Documents/Mozilla/projects/node_modules/@babel/preset-env/lib/index.js',
                  {
                    debug: false,
                    useBuiltIns: 'entry',
                    targets: {
                      browsers: [
                        'last 2 Chrome versions',
                        'last 2 Firefox versions',
                        'last 2 Edge versions',
                        'last 2 Opera versions',
                        'last 2 Safari versions',
                        'last 2 iOS versions'
                      ]
                    }
                  }
                ]
              ],
              plugins: [
                '/Users/haali/Documents/Mozilla/projects/taskcluster-client-web/node_modules/@neutrinojs/library/node_modules/@babel/plugin-syntax-dynamic-import/lib/index.js',
                '/Users/haali/Documents/Mozilla/projects/taskcluster-client-web/node_modules/@babel/plugin-proposal-class-properties/lib/index.js'
              ]
            }
          }
        ]

[edmorley]

That's not what I get - check for typos? (I also refreshed yarn.lock, try that too?)

yarn run v1.13.0
$ C:\Users\Ed\src\taskcluster-client-web\node_modules\.bin\neutrino --inspect
{
  mode: 'development',
  devtool: 'inline-source-map',
  target: 'web',
  context: 'C:\\Users\\Ed\\src\\taskcluster-client-web',
  stats: {
    children: false,
    entrypoints: false,
    modules: false
  },
  node: {
    Buffer: false,
    fs: 'empty',
    tls: 'empty'
  },
  output: {
    path: 'C:\\Users\\Ed\\src\\taskcluster-client-web\\build',
    library: 'taskcluster',
    libraryTarget: 'umd',
    umdNamedDefine: true
  },
  resolve: {
    alias: {
      hawk: 'hawk/dist/browser.js'
    },
    extensions: [
      '.wasm',
      '.mjs',
      '.jsx',
      '.js',
      '.json'
    ]
  },
  module: {
    rules: [
      /* neutrino.config.module.rule('compile') */
      {
        test: /\.(mjs|jsx|js)$/,
        include: [
          'C:\\Users\\Ed\\src\\taskcluster-client-web\\src',
          'C:\\Users\\Ed\\src\\taskcluster-client-web\\test'
        ],
        use: [
          /* neutrino.config.module.rule('compile').use('babel') */
          {
            loader: 'C:\\Users\\Ed\\src\\taskcluster-client-web\\node_modules\\babel-loader\\lib\\index.js',
            options: {
              cacheDirectory: true,
              babelrc: false,
              configFile: false,
              presets: [
                [
                  'C:\\Users\\Ed\\src\\taskcluster-client-web\\node_modules\\@babel\\preset-env\\lib\\index.js',
                  {
                    debug: false,
                    useBuiltIns: 'entry',
                    targets: {
                      browsers: [
                        'last 2 Chrome versions',
                        'last 2 Firefox versions',
                        'last 2 Edge versions',
                        'last 2 Opera versions',
                        'last 2 Safari versions',
                        'last 2 iOS versions'
                      ]
                    }
                  }
                ]
              ],
              plugins: [
                'C:\\Users\\Ed\\src\\taskcluster-client-web\\node_modules\\@babel\\plugin-syntax-dynamic-import\\lib\\index.js',
                'C:\\Users\\Ed\\src\\taskcluster-client-web\\node_modules\\@babel\\plugin-proposal-class-properties\\lib\\index.js'
              ]
            }
          }
        ]
      }
    ]
  },
  entry: {
    index: [
      'C:\\Users\\Ed\\src\\taskcluster-client-web\\src\\index'
    ]
  }
}
Done in 0.30s.

[edmorley]

neutrino --inspect --mode test shows:

This wouldn't have affected the merge, but note that --mode test is not a valid mode, and would fail the webpack schema validation if used outside of --inspect. Use:
NODE_ENV=test yarn neutrino --inspect
(https://master.neutrinojs.org/usage/#inspecting-the-generated-webpack-config)

[edmorley]

It's because they are in different directories:
/Users/haali/Documents/Mozilla/projects/node_modules/
vs
/Users/haali/Documents/Mozilla/projects/taskcluster-client-web/node_modules/

I'm presuming due to usage of NODE_PATH or similar (or a hand copy-paste typo?).

[helfi92]

Refreshing yarn.lock seems to fix it.

[edmorley]

I've split the IE9/polyfills part out to #1279, leaving this issue to just be about @neutrinojs/library not playing nicely with @neutrinojs/karma due to the externals option.

There are two ways to fix this (for more context see: #1251 (comment)):

1. Make @neutrinojs/library disable externals when NODE_ENV==='test':
2. Make @neutrinojs/karma disable externals when the karma output handler is invoked

Arguments for (1) are:

• It was the way it was implemented in Neutrino 8
• Test frameworks other than Karma don't use webpack at all (they invoke Babel directly), so clearing externals is of no consequence to them
• The resultant webpack config is easier to debug, since it's not hiding behaviour under the karma output handler (which isn't in the output shown when using neutrino --inspect)

Arguments for (2) are:

• All of the other testing presets are for test runners that run under node, so are fine with the externals option, so since karma is the one that needs externals to not be set, perhaps it should be the one to implement the workaround?

I think I'm leaning towards (1).

@neutrinojs/core-contributors - thoughts?

[edmorley]

I think I'm leaning towards (1).
@neutrinojs/core-contributors - thoughts?

[eliperelman]

I think (1) is acceptable. I just pushed a fix for the double externals call in the library preset, so I'll get a patch for (1) as well.