Skip to content
This repository was archived by the owner on Aug 7, 2021. It is now read-only.

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

Merged
merged 5 commits into from
Feb 1, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
54 changes: 54 additions & 0 deletions index.spec.ts
Original file line number Diff line number Diff line change
@@ -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(', ')}`);
});
});
});
});
});
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
7 changes: 2 additions & 5 deletions templates/webpack.angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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));
Expand Down
113 changes: 113 additions & 0 deletions templates/webpack.config.spec.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
});
});
});
});
4 changes: 1 addition & 3 deletions templates/webpack.javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 1 addition & 3 deletions templates/webpack.typescript.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 1 addition & 3 deletions templates/webpack.vue.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down