fix: Stop bundling dependencies with slashes #1634
Conversation
External dependencies should not be bundled, because they are already referenced via "dependencies" in package.json. Before this patch, external modules with slashes in their identifier were bundled anyway because there is no directory with that name in the `node_modules` directory (which in turn prevents them from being added to the `externals` object in the webpack config). This is fixed by looking up dependencies in `package.json` instead.
Rob--W
force-pushed
the
fix-webpack-externals
branch
from
38da0f3 to
4854788
Compare
1 similar comment
There was a problem hiding this comment.
Thanks @Rob--W
This looks good to me.
It would be great to also tweak how our "production mode" function tests run, so that we make sure that we trigger this kind of issue from the CI jobs.
I briefly took a look and the following changes seems to be enough to make TEST_PRODUCTION_MODE=1 npm run test-function to trigger the Cannot find module 'es6-promise' error without this fix (and to pass as expected with this fix applied):
diff --git a/scripts/lib/config.js b/scripts/lib/config.js
index 331262a..5c11b85 100644
--- a/scripts/lib/config.js
+++ b/scripts/lib/config.js
@@ -1,14 +1,13 @@
module.exports = {
clean: ['dist/*'],
copy: {
- productionModeArtifacts: {
+ productionModeAssets: {
src: [
'package.json',
'package-lock.json',
'dist/**',
'bin/**',
],
- dest: './artifacts/production',
},
},
watch: {
diff --git a/scripts/test-functional b/scripts/test-functional
index 0fac1a2..f636f93 100755
--- a/scripts/test-functional
+++ b/scripts/test-functional
@@ -1,6 +1,9 @@
#!/usr/bin/env node
+const path = require('path');
+
const shell = require('shelljs');
+const tmp = require('tmp');
const config = require('./lib/config');
const {mochaFunctional} = require('./lib/mocha');
@@ -9,36 +12,32 @@ shell.set('-e');
const testProductionMode = process.env.TEST_PRODUCTION_MODE === '1';
-let execBuildOptions = {};
let execMochaOptions = {};
+shell.exec('npm run build', testProductionMode ? {
+ env: {
+ ...process.env,
+ NODE_ENV: 'production',
+ },
+} : {});
+
if (testProductionMode) {
- execBuildOptions = {
- env: {
- ...process.env,
- NODE_ENV: 'production',
- },
- };
+ const destDir = tmp.tmpNameSync();
+ const packageDir = tmp.tmpNameSync();
execMochaOptions = {
env: {
...process.env,
- TEST_WEB_EXT_BIN: './artifacts/production/bin/web-ext',
+ TEST_WEB_EXT_BIN: path.join(destDir, 'node_modules', '.bin', 'web-ext'),
},
};
-}
-
-shell.exec('npm run build', execBuildOptions);
-if (testProductionMode) {
shell.echo('\nPreparing web-ext production mode environment...\n');
- shell.rm('-rf', config.copy.productionModeArtifacts.dest);
- shell.mkdir('-p', config.copy.productionModeArtifacts.dest);
- shell.cp('-rf', config.copy.productionModeArtifacts.src,
- config.copy.productionModeArtifacts.dest);
-
- shell.pushd(config.copy.productionModeArtifacts.dest);
- shell.exec('npm install --production');
+ shell.rm('-rf', destDir, packageDir);
+ shell.mkdir('-p', destDir, packageDir);
+ shell.cp('-rf', config.copy.productionModeAssets.src, packageDir);
+ shell.pushd(destDir);
+ shell.exec(`npm install --production --legacy-bundling ${packageDir}`);
shell.popd();
shell.echo('\nProduction mode environment successfully created.\n');
}
|
|
I agree, but it does still concern me that we are not able to catch this kind of dependency loading issues. I'm fine with proceeding with merging the fix as is, but it would be great (at least in a follow up) to look into trigger this kind of issue (that's the reason why we have a "test-functional running in production mode" as part of our CI jobs, "having to test this manually" kills its purpose) at least while there is still a way (e.g. using an additional npm option) to trigger it. |
My concern with using
Could you open a new PR with your proposed patch (and try to address my review feedback if you agree that it's a potential issue)? |
What I was proposing is to start from the set of changes linked in my previous comments to prepare an additional 'legacy-bundling production mode" test step in the CI job, but if you prefer I can take a look into it as part of a follow up. |
See commit message.
Fixes #1629