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

fix: handle relative requests from inside an external module #918

Closed
wants to merge 1 commit into from
Closed

fix: handle relative requests from inside an external module #918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 21 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,32 @@ exports.getSourceMapFilename = (hiddenSourceMap, appFolderPath, outputPath) => {
*/
exports.getConvertedExternals = (externals) => {
const modifiedExternals = (externals || []).map((e) => {
return new RegExp(`^${e}((/.*)|$)`);
return new RegExp(`((node_modules\/)|^)${e}((/.*)|$)`);
});

return modifiedExternals;
};

exports.getExternalsHandler = (externalRegexps) => {
if (!Array.isArray(externalRegexps) || externalRegexps.some(regExp => !(regExp instanceof RegExp))) {
throw new Error(`Expected Array of regular expressions. Got '${externalRegexps}'.`);
}

const handler = (context, request, callback) => {
let nonRelativeRequest = request;
if (request && request.startsWith(".")) {
nonRelativeRequest = path.join(context, request);
}

if (externalRegexps.some(regExp => regExp.test(nonRelativeRequest))) {
return callback(null, 'commonjs ' + request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if here you shouldn't use nonRelativeRequest.

}
callback();
}

return handler;
}

const sanitize = name => name
.split("")
.filter(char => /[a-zA-Z0-9]/.test(char))
Expand Down
107 changes: 104 additions & 3 deletions index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,107 @@
import { getConvertedExternals, getEntryPathRegExp } from './index';
import { getConvertedExternals, getEntryPathRegExp, getExternalsHandler } from './index';
const path = require("path");

describe('index', () => {
describe('getExternalsHandler', () => {
it('throws an error when the argument is not an array of regular expressions', () => {
expect(() => getExternalsHandler()).toThrowError();
expect(() => getExternalsHandler(32)).toThrowError();
expect(() => getExternalsHandler(null)).toThrowError();
expect(() => getExternalsHandler("asd")).toThrowError();
expect(() => getExternalsHandler([{}])).toThrowError();
expect(() => getExternalsHandler(/((node_modules\/)|^)nativescript-vue((\/.*)|$)/)).toThrowError();
});

const externalContextTestCases = [
{
name: 'request for another external module from node_modules',
input: 'nativescript-angular',
expectedIsExternal: true
},
{
name: 'request for the same module from node_modules',
input: 'nativescript-vue',
expectedIsExternal: true
},
{
name: 'request for a relative file in the same external module',
input: './vue-index',
expectedIsExternal: true
},
{
name: 'request for a relative file outside the external module',
input: '../another-non-external-module/another-index',
expectedIsExternal: false
},
{
name: 'request for an absolute local path',
input: '/nativescript-vue',
expectedIsExternal: false
}
];

externalContextTestCases.forEach(testCase => {
it(`returns handler which matches requests inside an external module - ${testCase.name}`, (done) => {
const handler = getExternalsHandler([/((node_modules\/)|^)nativescript-vue((\/.*)|$)/, /((node_modules\/)|^)nativescript-angular((\/.*)|$)/]);
handler("./node_modules/nativescript-vue/", testCase.input, (error, externalLink) => {
if (testCase.expectedIsExternal) {
expect(error).toBeNull();
expect(externalLink).toBeDefined();
} else {
expect(error).toBeUndefined();
expect(externalLink).toBeUndefined();
}
done();
});
});
});


const nonExternalContextTestCases = [
{
name: 'request for an external module from node_modules',
input: 'nativescript-vue',
expectedIsExternal: true
},
{
name: 'request for a relative file from an external module',
input: '../node_modules/nativescript-vue/vue-index',
expectedIsExternal: true
},
{
name: 'request for a relative file inside the app',
input: './app-module',
expectedIsExternal: false
},
{
name: 'request for a relative file from a non external module',
input: '../node_modules/another-non-external-module/another-index',
expectedIsExternal: false
},
{
name: 'request for an absolute local path',
input: '/nativescript-vue',
expectedIsExternal: false
}
];

nonExternalContextTestCases.forEach(testCase => {
it(`returns handler which matches requests from inside the app folder - ${testCase.name}`, (done) => {
const handler = getExternalsHandler([/((node_modules\/)|^)nativescript-vue((\/.*)|$)/]);
handler("./app/", testCase.input, (error, externalLink) => {
if (testCase.expectedIsExternal) {
expect(error).toBeNull();
expect(externalLink).toBeDefined();
} else {
expect(error).toBeUndefined();
expect(externalLink).toBeUndefined();
}
done();
});
});
});
});

describe('getConvertedExternals', () => {
it('returns empty array when nullable is passed', () => {
const actualResult = getConvertedExternals(null);
Expand All @@ -11,11 +111,11 @@ describe('index', () => {
const testCases = [
{
input: ['nativescript-vue'],
expectedOutput: [/^nativescript-vue((\/.*)|$)/]
expectedOutput: [/((node_modules\/)|^)nativescript-vue((\/.*)|$)/]
},
{
input: ['nativescript-vue', 'nativescript-angular'],
expectedOutput: [/^nativescript-vue((\/.*)|$)/, /^nativescript-angular((\/.*)|$)/]
expectedOutput: [/((node_modules\/)|^)nativescript-vue((\/.*)|$)/, /((node_modules\/)|^)nativescript-angular((\/.*)|$)/]
}
];

Expand All @@ -29,6 +129,7 @@ describe('index', () => {
[
'nativescript-vue',
'nativescript-vue/subdir',
'./node_modules/nativescript-vue/subdir',
'nativescript-vue/subdir/subdir-2'
].forEach(testString => {
const regExpsExternals = getConvertedExternals(testCase.input);
Expand Down
2 changes: 1 addition & 1 deletion templates/webpack.angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ module.exports = env => {
const config = {
mode: uglify ? "production" : "development",
context: appFullPath,
externals,
externals: nsWebpack.getExternalsHandler(externals),
watchOptions: {
ignored: [
appResourcesFullPath,
Expand Down
30 changes: 23 additions & 7 deletions templates/webpack.config.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ class TerserJsStub {
}
};

let providedExternalRegExps: any;
const fakeExternalsHandler = () => null;
const testGetExternalsHandler = (regExps) => {
providedExternalRegExps = regExps;
return fakeExternalsHandler;
};
const nativeScriptDevWebpack = {
GenerateBundleStarterPlugin: EmptyClass,
WatchStateLoggerPlugin: EmptyClass,
Expand All @@ -32,6 +38,7 @@ const nativeScriptDevWebpack = {
getResolver: () => null,
getEntryPathRegExp: () => null,
getConvertedExternals: nsWebpackIndex.getConvertedExternals,
getExternalsHandler: testGetExternalsHandler,
getSourceMapFilename: nsWebpackIndex.getSourceMapFilename
};

Expand Down Expand Up @@ -94,41 +101,50 @@ describe('webpack.config.js', () => {
describe(`verify externals for webpack.${type}.js (${platform})`, () => {
afterEach(() => {
nativeScriptDevWebpack.getConvertedExternals = nsWebpackIndex.getConvertedExternals;
nativeScriptDevWebpack.getExternalsHandler = testGetExternalsHandler;
});

it('returns empty array when externals are not passed', () => {
const input = getInput({ platform });
const config = webpackConfig(input);
expect(config.externals).toEqual([]);
expect(config.externals).toEqual(fakeExternalsHandler);
expect(providedExternalRegExps).toEqual([]);
});

it('calls getConvertedExternals to convert externals', () => {
let isCalled = false;
let isConvertExternalsCalled = false;
let isGetExternalsHandlerCalled = false;
nativeScriptDevWebpack.getConvertedExternals = () => {
isCalled = true;
isConvertExternalsCalled = true;
return [];
};
nativeScriptDevWebpack.getExternalsHandler = () => {
isGetExternalsHandlerCalled = true;
return () => null;
};

const input = getInput({ platform, externals: ['nativescript-vue'] });
webpackConfig(input);
expect(isCalled).toBe(true, 'Webpack.config.js must use the getConvertedExternals method');
expect(isConvertExternalsCalled).toBe(true, 'Webpack.config.js must use the getConvertedExternals method');
expect(isGetExternalsHandlerCalled).toBe(true, 'Webpack.config.js must use the getExternalsHandler method');
});

[
{
input: ['nativescript-vue'],
expectedOutput: [/^nativescript-vue((\/.*)|$)/]
expectedOutput: [/((node_modules\/)|^)nativescript-vue((\/.*)|$)/]
},
{
input: ['nativescript-vue', 'nativescript-angular'],
expectedOutput: [/^nativescript-vue((\/.*)|$)/, /^nativescript-angular((\/.*)|$)/]
expectedOutput: [/((node_modules\/)|^)nativescript-vue((\/.*)|$)/, /((node_modules\/)|^)nativescript-angular((\/.*)|$)/]
},
].forEach(testCase => {
const input = getInput({ platform, externals: testCase.input });

it(`are correct regular expressions, for input ${testCase.input}`, () => {
const config = webpackConfig(input);
expect(config.externals).toEqual(testCase.expectedOutput);
expect(config.externals).toEqual(fakeExternalsHandler);
expect(providedExternalRegExps).toEqual(testCase.expectedOutput);
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion templates/webpack.javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ module.exports = env => {
const config = {
mode: uglify ? "production" : "development",
context: appFullPath,
externals,
externals: nsWebpack.getExternalsHandler(externals),
watchOptions: {
ignored: [
appResourcesFullPath,
Expand Down
2 changes: 1 addition & 1 deletion templates/webpack.typescript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ module.exports = env => {
const config = {
mode: uglify ? "production" : "development",
context: appFullPath,
externals,
externals: nsWebpack.getExternalsHandler(externals),
watchOptions: {
ignored: [
appResourcesFullPath,
Expand Down
2 changes: 1 addition & 1 deletion templates/webpack.vue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ module.exports = env => {
const config = {
mode: mode,
context: appFullPath,
externals,
externals: nsWebpack.getExternalsHandler(externals),
watchOptions: {
ignored: [
appResourcesFullPath,
Expand Down