Skip to content

Commit

Permalink
Exclude contextual module references from cleaning dll task (elastic#…
Browse files Browse the repository at this point in the history
…38733) (elastic#40037)

* fix(NA): exclude contextual module references from cleaning dll tasks

* test(NA): fix tests for nodejs modules webpack dll build task.

* chore(NA): add test for non accessible files.

* chore(NA): apply requested changed on the PR review.

* test(NA): fix for test cases

* fix(na): circular call on isFileAccessible implementation.
  • Loading branch information
mistic authored Jul 1, 2019
1 parent d63c225 commit e5c5456
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 11 deletions.
11 changes: 11 additions & 0 deletions src/dev/build/lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ export function assertAbsolute(path) {
}
}

export function isFileAccessible(path) {
assertAbsolute(path);

try {
fs.accessSync(path);
return true;
} catch (e) {
return false;
}
}

function longInspect(value) {
return inspect(value, {
maxArrayLength: Infinity
Expand Down
1 change: 1 addition & 0 deletions src/dev/build/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export {
deleteAll,
deleteEmptyFolders,
compress,
isFileAccessible,
} from './fs';
export { scanDelete } from './scan_delete';
export { scanCopy } from './scan_copy';
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,9 @@ export const CleanClientModulesOnDLLTask = {

// Get dll entries filtering out the ones
// from any whitelisted module
const dllEntries = await getDllEntries(dllManifestPath, whiteListedModules);
const dllEntries = await getDllEntries(dllManifestPath, whiteListedModules, baseDir);

for (const relativeEntryPath of dllEntries) {
if (relativeEntryPath.includes('@elastic/eui')) {
continue;
}

const entryPath = `${baseDir}/${relativeEntryPath}`;

// Clean a module included into the dll
Expand Down
23 changes: 19 additions & 4 deletions src/dev/build/tasks/nodejs_modules/webpack_dll.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,17 @@
* under the License.
*/

import { deleteAll, read, write } from '../../lib';
import { dirname, sep, relative } from 'path';
import { deleteAll, isFileAccessible, read, write } from '../../lib';
import { dirname, sep, relative, resolve } from 'path';
import pkgUp from 'pkg-up';
import globby from 'globby';

export async function getDllEntries(manifestPath, whiteListedModules) {
function checkDllEntryAccess(entry, baseDir = '') {
const resolvedPath = baseDir ? resolve(baseDir, entry) : entry;
return isFileAccessible(resolvedPath);
}

export async function getDllEntries(manifestPath, whiteListedModules, baseDir = '') {
const manifest = JSON.parse(await read(manifestPath));

if (!manifest || !manifest.content) {
Expand All @@ -46,7 +51,17 @@ export async function getDllEntries(manifestPath, whiteListedModules) {
const isWhiteListed = whiteListedModules.some(nonEntry => entry.includes(`node_modules${sep}${nonEntry}${sep}`));
const isNodeModule = entry.includes('node_modules');

return !isWhiteListed && isNodeModule;
// NOTE: when using dynamic imports on webpack the entry paths could be created
// with special context module (ex: lazy recursive) values over directories that are not real files
// and only exists in runtime, so we need to check if the entry is a real file.
// We found that problem through the issue https://github.com/elastic/kibana/issues/38481
//
// More info:
// https://github.com/webpack/webpack/blob/master/examples/code-splitting-harmony/README.md
// https://webpack.js.org/guides/dependency-management/#require-with-expression
const isAccessible = checkDllEntryAccess(entry, baseDir);

return !isWhiteListed && isNodeModule && isAccessible;
});
}

Expand Down
21 changes: 19 additions & 2 deletions src/dev/build/tasks/nodejs_modules/webpack_dll.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,12 @@
* under the License.
*/

import { read } from '../../lib';
import { isFileAccessible, read } from '../../lib';
import { getDllEntries } from './webpack_dll';

jest.mock('../../lib', () => ({
read: jest.fn()
read: jest.fn(),
isFileAccessible: jest.fn(),
}));

const manifestContentMock = JSON.stringify({
Expand Down Expand Up @@ -49,13 +50,29 @@ describe('Webpack DLL Build Tasks Utils', () => {
it('should get dll entries correctly', async () => {
read.mockImplementationOnce(async () => manifestContentMock);

isFileAccessible.mockImplementation(() => true);

const mockManifestPath = '/mock/mock_dll_manifest.json';
const mockModulesWhitelist = [ 'dep1' ];
const dllEntries = await getDllEntries(mockManifestPath, mockModulesWhitelist);

expect(dllEntries).toEqual(expect.arrayContaining(['/mock/node_modules/dep2', '/mock/node_modules/dep3']));
});

it('should only include accessible files', async () => {
read.mockImplementationOnce(async () => manifestContentMock);

isFileAccessible.mockImplementation(() => false);

const mockManifestPath = '/mock/mock_dll_manifest.json';
const mockModulesWhitelist = [ 'dep1' ];
const dllEntries = await getDllEntries(mockManifestPath, mockModulesWhitelist);

isFileAccessible.mockRestore();

expect(dllEntries.length).toEqual(0);
});

it('should throw an error for no manifest file', async () => {
read.mockImplementationOnce(async () => noManifestMock);

Expand Down

0 comments on commit e5c5456

Please sign in to comment.