fix: some packages are treated as externals when they shouldn't (#771)

* fix: some packages are treated as externals when they shouldn't

In case `externals` are passed to webpack.config.js, they are converted to regular expression. The idea is to detect all imports for this module, for example if it is `nativescript-vue`, we would like to make externals imports like `nativescript-vue/somesubdir/file1`.
However, the regular expression we construct also matches packages like `nativescript-vue-template`.
Fix the regular expressions in all template webpack.config.js files and add unit tests for them.
Add devDependency to `proxyquire` as the webpack configs that are tested, require some packages that are not available in node_modules of `nativescript-dev-webpack` (including the `nativescript-dev-webpack` itself). This is expected for the `webpack.config.js` files, as they should be placed in the NativeScript applications, so mocking the requires shouldn't be a problem in this case.

* chore: move externals generation to a common place

Move the conversion of regular expressions from `string[]` to `RegExp[]` to index.js file, so it can be reused in all webpack.config.js files.
Move most of the tests to the `index.spec.js` due to this change.

* chore: fix grammar errors

Co-Authored-By: rosen-vladimirov <[email protected]>

Author: rosen-vladimirov

Diff for: index.js

@@ -53,6 +53,25 @@ exports.getAppPath = (platform, projectDir) => {
     }
 };
 
+/**
+ * Converts an array of strings externals to an array of regular expressions.
+ * Input is an array of string, which we need to convert to regular expressions, so all imports for this module will be treated as externals.
+
+ * For example, in case we want nativescript-vue to be external, we will pass `["nativescript-vue"]`.
+ * If we pass it to webpack in this way, it will treat all `require("nativescript-vue")` as externals.
+ * However, you may import some file/subdir of the module, for example `require("nativescript-vue/somedir/file1")`.
+ * To treat this as external, we convert the strings to regular expressions, which makes webpack exclude all imports of the module.
+ * @param {string[]} externals Array of strings.
+ * @returns {RegExp[]} Array of regular expressions constructed from the input strings. In case the input is nullable, an empty array is returned.
+ */
+exports.getConvertedExternals = (externals) => {
+    const modifiedExternals = (externals || []).map((e) => {
+        return new RegExp(`^${e}((/.*)|$)`);
+    });
+
+    return modifiedExternals;
+};
+
 const sanitize = name => name
     .split("")
     .filter(char => /[a-zA-Z0-9]/.test(char))

Diff for: index.spec.ts

@@ -0,0 +1,54 @@
+import { getConvertedExternals } from './index';
+
+describe('index', () => {
+    describe('getConvertedExternals', () => {
+        it('returns empty array when nullable is passed', () => {
+            const actualResult = getConvertedExternals(null);
+            expect(actualResult).toEqual([]);
+        });
+
+        const testCases = [
+            {
+                input: ['nativescript-vue'],
+                expectedOutput: [/^nativescript-vue((\/.*)|$)/]
+            },
+            {
+                input: ['nativescript-vue', 'nativescript-angular'],
+                expectedOutput: [/^nativescript-vue((\/.*)|$)/, /^nativescript-angular((\/.*)|$)/]
+            }
+        ];
+
+        testCases.forEach(testCase => {
+            it('converts passed strings to regular expressions', () => {
+                const actualResult = getConvertedExternals(testCase.input);
+                expect(actualResult).toEqual(testCase.expectedOutput);
+            });
+
+            it(`returns regular expressions which match expected modules and their subdirs, for input ${testCase.input}`, () => {
+                [
+                    'nativescript-vue',
+                    'nativescript-vue/subdir',
+                    'nativescript-vue/subdir/subdir-2'
+                ].forEach(testString => {
+                    const regExpsExternals = getConvertedExternals(testCase.input);
+                    const result = regExpsExternals.some((regExp: RegExp) => !!regExp.exec(testString));
+                    expect(result).toBe(true, `String ${testString} does not match any of the regular expressions: ${regExpsExternals.join(', ')}`);
+                });
+            });
+
+            it(`returns regular expressions which does not match expected modules and their subdirs, for input ${testCase.input}`, () => {
+                [
+                    'nativescript-facebook',
+                    'nativescript-facebook/nativescript-vue',
+                    'main-plugin/subdir/nativescript-vue',
+                    'nativescript-vue-template-compiler',
+                    'nativescript-vue-template-compiler/subdir'
+                ].forEach(testString => {
+                    const regExpsExternals = getConvertedExternals(testCase.input);
+                    const result = regExpsExternals.some((regExp: RegExp) => !!regExp.exec(testString));
+                    expect(result).toBe(false, `String ${testString} matches some of the regular expressions: ${regExpsExternals.join(', ')}`);
+                });
+            });
+        });
+    });
+});

Diff for: package.json

@@ -100,11 +100,13 @@
   },
   "devDependencies": {
     "@ngtools/webpack": "~7.1.0",
-    "@types/jasmine": "^3.3.1",
+    "@types/jasmine": "^3.3.7",
     "@types/node": "^10.12.12",
+    "@types/proxyquire": "1.3.28",
     "conventional-changelog-cli": "^1.3.22",
     "jasmine": "^3.2.0",
     "jasmine-spec-reporter": "^4.2.1",
+    "proxyquire": "2.1.0",
     "typescript": "~3.1.1"
   }
 }

Diff for: templates/webpack.angular.js

@@ -46,11 +46,8 @@ module.exports = env => {
         sourceMap, // --env.sourceMap
         hmr, // --env.hmr,
     } = env;
-    env.externals = env.externals || [];
-    const externals = (env.externals).map((e) => { // --env.externals
-        return new RegExp(e + ".*");
-    });
 
+    const externals = nsWebpack.getConvertedExternals(env.externals);
     const appFullPath = resolve(projectRoot, appPath);
     const appResourcesFullPath = resolve(projectRoot, appResourcesPath);
 
@@ -65,7 +62,7 @@ module.exports = env => {
     // when "@angular/core" is external, it's not included in the bundles. In this way, it will be used
     // directly from node_modules and the Angular modules loader won't be able to resolve the lazy routes
     // fixes https://github.com/NativeScript/nativescript-cli/issues/4024
-    if (env.externals.indexOf("@angular/core") > -1) {
+    if (externals.indexOf("@angular/core") > -1) {
         const appModuleRelativePath = getMainModulePath(resolve(appFullPath, entryModule));
         if (appModuleRelativePath) {
             const appModuleFolderPath = dirname(resolve(appFullPath, appModuleRelativePath));

Diff for: templates/webpack.config.spec.ts

@@ -0,0 +1,113 @@
+import * as proxyquire from 'proxyquire';
+import * as nsWebpackIndex from '../index';
+// With noCallThru enabled, `proxyquire` will not fall back to requiring the real module to populate properties that are not mocked.
+// This allows us to mock packages that are not available in node_modules.
+// In case you want to enable fallback for a specific object, just add `'@noCallThru': false`.
+proxyquire.noCallThru();
+
+class EmptyClass { };
+
+const nativeScriptDevWebpack = {
+    GenerateBundleStarterPlugin: EmptyClass,
+    WatchStateLoggerPlugin: EmptyClass,
+    PlatformFSPlugin: EmptyClass,
+    getAppPath: () => 'app',
+    getEntryModule: () => 'EntryModule',
+    getResolver: () => null,
+    getConvertedExternals: nsWebpackIndex.getConvertedExternals
+};
+
+const emptyObject = {};
+
+const webpackConfigAngular = proxyquire('./webpack.angular', {
+    'nativescript-dev-webpack': nativeScriptDevWebpack,
+    'nativescript-dev-webpack/nativescript-target': emptyObject,
+    'nativescript-dev-webpack/transformers/ns-replace-bootstrap': emptyObject,
+    'nativescript-dev-webpack/transformers/ns-replace-lazy-loader': emptyObject,
+    'nativescript-dev-webpack/utils/ast-utils': emptyObject,
+    '@ngtools/webpack': {
+        AngularCompilerPlugin: EmptyClass
+    }
+});
+
+const webpackConfigTypeScript = proxyquire('./webpack.typescript', {
+    'nativescript-dev-webpack': nativeScriptDevWebpack,
+    'nativescript-dev-webpack/nativescript-target': emptyObject,
+});
+
+const webpackConfigJavaScript = proxyquire('./webpack.javascript', {
+    'nativescript-dev-webpack': nativeScriptDevWebpack,
+    'nativescript-dev-webpack/nativescript-target': emptyObject,
+});
+
+const webpackConfigVue = proxyquire('./webpack.vue', {
+    'nativescript-dev-webpack': nativeScriptDevWebpack,
+    'nativescript-dev-webpack/nativescript-target': emptyObject,
+    'vue-loader/lib/plugin': EmptyClass,
+    'nativescript-vue-template-compiler': emptyObject
+});
+
+describe('webpack.config.js', () => {
+    [
+        { type: 'javascript', webpackConfig: webpackConfigJavaScript },
+        { type: 'typescript', webpackConfig: webpackConfigTypeScript },
+        { type: 'angular', webpackConfig: webpackConfigAngular },
+        { type: 'vue', webpackConfig: webpackConfigVue }
+    ].forEach(element => {
+        const { type, webpackConfig } = element;
+
+        describe(`verify externals for webpack.${type}.js`, () => {
+            const getInput = (platform: string, externals: string[]) => {
+                const input: any = { externals };
+                input[platform] = true;
+                return input;
+            };
+
+            [
+                'android',
+                'ios'
+            ].forEach(platform => {
+                describe(`for ${platform}`, () => {
+                    afterEach(() => {
+                        nativeScriptDevWebpack.getConvertedExternals = nsWebpackIndex.getConvertedExternals;
+                    });
+
+                    it('returns empty array when externals are not passed', () => {
+                        const config = webpackConfig(getInput(platform, null));
+                        expect(config.externals).toEqual([]);
+                    });
+
+                    it('calls getConvertedExternals to convert externals', () => {
+                        let isCalled = false;
+                        nativeScriptDevWebpack.getConvertedExternals = () => {
+                            isCalled = true;
+                            return [];
+                        };
+
+                        const input = getInput(platform, ['nativescript-vue']);
+                        webpackConfig(input);
+                        expect(isCalled).toBe(true, 'Webpack.config.js must use the getConvertedExternals method');
+                    });
+
+                    [
+                        {
+                            input: ['nativescript-vue'],
+                            expectedOutput: [/^nativescript-vue((\/.*)|$)/]
+                        },
+                        {
+                            input: ['nativescript-vue', 'nativescript-angular'],
+                            expectedOutput: [/^nativescript-vue((\/.*)|$)/, /^nativescript-angular((\/.*)|$)/]
+                        },
+                    ].forEach(testCase => {
+                        const input = getInput(platform, testCase.input);
+
+                        it(`are correct regular expressions, for input ${testCase.input}`, () => {
+                            const config = webpackConfig(input);
+                            expect(config.externals).toEqual(testCase.expectedOutput);
+                        });
+                    });
+                });
+            });
+        });
+    });
+});

Diff for: templates/webpack.javascript.js

@@ -42,9 +42,7 @@ module.exports = env => {
         sourceMap, // --env.sourceMap
         hmr, // --env.hmr,
     } = env;
-    const externals = (env.externals || []).map((e) => { // --env.externals
-        return new RegExp(e + ".*");
-    });
+    const externals = nsWebpack.getConvertedExternals(env.externals);
 
     const appFullPath = resolve(projectRoot, appPath);
     const appResourcesFullPath = resolve(projectRoot, appResourcesPath);

Diff for: templates/webpack.typescript.js

@@ -42,9 +42,7 @@ module.exports = env => {
         sourceMap, // --env.sourceMap
         hmr, // --env.hmr,
     } = env;
-    const externals = (env.externals || []).map((e) => { // --env.externals
-        return new RegExp(e + ".*");
-    });
+    const externals = nsWebpack.getConvertedExternals(env.externals);
 
     const appFullPath = resolve(projectRoot, appPath);
     const appResourcesFullPath = resolve(projectRoot, appResourcesPath);

Diff for: templates/webpack.vue.js

@@ -46,9 +46,7 @@ module.exports = env => {
         hmr, // --env.hmr
     } = env;
 
-    const externals = (env.externals || []).map((e) => { // --env.externals
-        return new RegExp(e + ".*");
-    });
+    const externals = nsWebpack.getConvertedExternals(env.externals);
 
     const mode = production ? "production" : "development"