Bug fix: Respect externals options in library plugin #999
Conversation
| @@ -56,7 +56,7 @@ module.exports = (neutrino, opts = {}) => { | |||
| neutrino.config | |||
| .when(hasSourceMap, () => neutrino.use(banner)) | |||
| .devtool('source-map') | |||
| .externals([nodeExternals()]) | |||
| .externals([nodeExternals(options.externals)]) | |||
There was a problem hiding this comment.
options.externals isn't defined by default, so this will be calling nodeExternals(undefined). This is currently handled ok, due to:
https://github.com/liady/webpack-node-externals/blob/cfa1c5b33752fb8cb72360167ae89b23816cdefe/index.js#L22
...however it doesn't feel right to be relying on an implementation detail that they could possibly change in a minor version.
As such, perhaps the implementation should copy what @neutrinojs/react-components does? (With optional cleanup there, since the current opts.externals !== false && {} appears overly complex, unless I'm mis-remembering how deepmerge handles merging different data types.)
There was a problem hiding this comment.
Thank you for updating the PR - it's missing the .when() of the react-components implementation though?
https://github.com/mozilla-neutrino/neutrino-dev/blob/ed21fa6074354609389710f051286356b073cb4e/packages/react-components/index.js#L53
Previously, externals was ignored. This passes along the options to webpack-node-externals. Fixes #987
|
OK, I've gone ahead and updated the code to use the same logic that react-components is using. Let me know if there's anything else that needs to be done before this is merged. |
|
Hi! Travis is failing due to an unrelated Jest/JSDom regression (jestjs/jest#6766). We're holding off merging any more things to |
|
@edmorley OK. I ran into that Jest issue in some of my projects. The fix is to set the environment, like so: |
|
Whilst that would work around the issue, it seems like this should be fixed in Jest, so once they do so, we'd want to remove the workaround again anyway. |
|
I understand. But shouldn't neutrino override |
Ah good point! So the default of In the meantime, a new Jest minor version has been released, which fixes the test failures - I've retriggered and Travis is now green :-) |
| @@ -56,7 +56,7 @@ module.exports = (neutrino, opts = {}) => { | |||
| neutrino.config | |||
| .when(hasSourceMap, () => neutrino.use(banner)) | |||
| .devtool('source-map') | |||
| .externals([nodeExternals()]) | |||
| .externals([nodeExternals(options.externals)]) | |||
There was a problem hiding this comment.
Thank you for updating the PR - it's missing the .when() of the react-components implementation though?
https://github.com/mozilla-neutrino/neutrino-dev/blob/ed21fa6074354609389710f051286356b073cb4e/packages/react-components/index.js#L53
|
@edmorley Sorry for the delay in my response. I'm not able to work on this more right now, as I'm no longer using neutrino. I'd be happy if someone else brought this one home. |
|
Continuing in #1040, thank you so much for your help here! |
Previously, externals was ignored. This passes along
the options to webpack-node-externals.
Fixes #987
I verified that this fix in a project I was working on. Without this change,
externals.whitelisthad no effect. With this change, whitelisted modules get bundled into the output file.