Skip to content

Commit

Permalink
fix(ngcc): do not crash on overlapping entry-points (#36083)
Browse files Browse the repository at this point in the history 
When two entry-points overlap, ngcc may attempt to process some
files twice. Previously, when this occured ngcc would just exit with an
error preventing any other entry-points from being processed.

This commit changes ngcc so that if `errorOnFailedEntryPoint` is false, it will
simply log an error and continue to process entry-points. This is useful when
ngcc is processing the entire node_modules folder and there are some invalid
entry-points that the project doesn't actually use.

PR Close #36083
  • Loading branch information
@petebacondarwin @AndrewKushnir
petebacondarwin authored and AndrewKushnir committed Mar 18, 2020
1 parent ff665b9 commit c9f554c
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 34 deletions.
12 changes: 7 additions & 5 deletions packages/compiler-cli/ngcc/src/main.ts
View file
Expand Up @@ -277,7 +277,8 @@ export function mainNgcc({basePath, targetEntryPointPath,

// The function for creating the `compile()` function.
const createCompileFn: CreateCompileFn = onTaskCompleted => {
const fileWriter = getFileWriter(fileSystem, pkgJsonUpdater, createNewEntryPointFormats);
const fileWriter = getFileWriter(
fileSystem, logger, pkgJsonUpdater, createNewEntryPointFormats, errorOnFailedEntryPoint);
const transformer = new Transformer(fileSystem, logger);

return (task: Task) => {
Expand Down Expand Up @@ -362,10 +363,11 @@ function getPackageJsonUpdater(inParallel: boolean, fs: FileSystem): PackageJson
}

function getFileWriter(
fs: FileSystem, pkgJsonUpdater: PackageJsonUpdater,
createNewEntryPointFormats: boolean): FileWriter {
return createNewEntryPointFormats ? new NewEntryPointFileWriter(fs, pkgJsonUpdater) :
new InPlaceFileWriter(fs);
fs: FileSystem, logger: Logger, pkgJsonUpdater: PackageJsonUpdater,
createNewEntryPointFormats: boolean, errorOnFailedEntryPoint: boolean): FileWriter {
return createNewEntryPointFormats ?
new NewEntryPointFileWriter(fs, logger, errorOnFailedEntryPoint, pkgJsonUpdater) :
new InPlaceFileWriter(fs, logger, errorOnFailedEntryPoint);
}

function getTaskQueue(
Expand Down
26 changes: 18 additions & 8 deletions packages/compiler-cli/ngcc/src/writing/in_place_file_writer.ts
View file
@@ -1,4 +1,3 @@

/**
* @license
* Copyright Google Inc. All Rights Reserved.
Expand All @@ -7,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {FileSystem, absoluteFrom, dirname} from '../../../src/ngtsc/file_system';
import {Logger} from '../logging/logger';
import {EntryPointJsonProperty} from '../packages/entry_point';
import {EntryPointBundle} from '../packages/entry_point_bundle';
import {FileToWrite} from '../rendering/utils';
Expand All @@ -18,7 +18,9 @@ export const NGCC_BACKUP_EXTENSION = '.__ivy_ngcc_bak';
* a back-up of the original file with an extra `.__ivy_ngcc_bak` extension.
*/
export class InPlaceFileWriter implements FileWriter {
constructor(protected fs: FileSystem) {}
constructor(
protected fs: FileSystem, protected logger: Logger,
protected errorOnFailedEntryPoint: boolean) {}

writeBundle(
_bundle: EntryPointBundle, transformedFiles: FileToWrite[],
Expand All @@ -30,12 +32,20 @@ export class InPlaceFileWriter implements FileWriter {
this.fs.ensureDir(dirname(file.path));
const backPath = absoluteFrom(`${file.path}${NGCC_BACKUP_EXTENSION}`);
if (this.fs.exists(backPath)) {
throw new Error(
`Tried to overwrite ${backPath} with an ngcc back up file, which is disallowed.`);
}
if (this.fs.exists(file.path)) {
this.fs.moveFile(file.path, backPath);
if (this.errorOnFailedEntryPoint) {
throw new Error(
`Tried to overwrite ${backPath} with an ngcc back up file, which is disallowed.`);
} else {
this.logger.error(
`Tried to write ${backPath} with an ngcc back up file but it already exists so not writing, nor backing up, ${file.path}.\n` +
`This error may be because two or more entry-points overlap and ngcc has been asked to process some files more than once.\n` +
`You should check other entry-points in this package and set up a config to ignore any that you are not using.`);
}
} else {
if (this.fs.exists(file.path)) {
this.fs.moveFile(file.path, backPath);
}
this.fs.writeFile(file.path, file.contents);
}
this.fs.writeFile(file.path, file.contents);
}
}
7 changes: 6 additions & 1 deletion packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts
View file
Expand Up @@ -8,6 +8,7 @@
*/
import {AbsoluteFsPath, FileSystem, absoluteFromSourceFile, dirname, join, relative} from '../../../src/ngtsc/file_system';
import {isDtsPath} from '../../../src/ngtsc/util/src/typescript';
import {Logger} from '../logging/logger';
import {EntryPoint, EntryPointJsonProperty} from '../packages/entry_point';
import {EntryPointBundle} from '../packages/entry_point_bundle';
import {FileToWrite} from '../rendering/utils';
Expand All @@ -27,7 +28,11 @@ export const NGCC_PROPERTY_EXTENSION = '_ivy_ngcc';
* `InPlaceFileWriter`).
*/
export class NewEntryPointFileWriter extends InPlaceFileWriter {
constructor(fs: FileSystem, private pkgJsonUpdater: PackageJsonUpdater) { super(fs); }
constructor(
fs: FileSystem, logger: Logger, errorOnFailedEntryPoint: boolean,
private pkgJsonUpdater: PackageJsonUpdater) {
super(fs, logger, errorOnFailedEntryPoint);
}

writeBundle(
bundle: EntryPointBundle, transformedFiles: FileToWrite[],
Expand Down
49 changes: 37 additions & 12 deletions packages/compiler-cli/ngcc/test/writing/in_place_file_writer_spec.ts
View file
Expand Up @@ -10,6 +10,7 @@ import {runInEachFileSystem} from '../../../src/ngtsc/file_system/testing';
import {loadTestFiles} from '../../../test/helpers';
import {EntryPointBundle} from '../../src/packages/entry_point_bundle';
import {InPlaceFileWriter} from '../../src/writing/in_place_file_writer';
import {MockLogger} from '../helpers/mock_logger';

runInEachFileSystem(() => {
describe('InPlaceFileWriter', () => {
Expand All @@ -24,13 +25,15 @@ runInEachFileSystem(() => {
{name: _('/package/path/folder-1/file-2.js'), contents: 'ORIGINAL FILE 2'},
{name: _('/package/path/folder-2/file-3.js'), contents: 'ORIGINAL FILE 3'},
{name: _('/package/path/folder-2/file-4.js'), contents: 'ORIGINAL FILE 4'},
{name: _('/package/path/already-backed-up.js'), contents: 'ORIGINAL ALREADY BACKED UP'},
{name: _('/package/path/already-backed-up.js.__ivy_ngcc_bak'), contents: 'BACKED UP'},
]);
});

it('should write all the FileInfo to the disk', () => {
const fs = getFileSystem();
const fileWriter = new InPlaceFileWriter(fs);
const logger = new MockLogger();
const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ true);
fileWriter.writeBundle({} as EntryPointBundle, [
{path: _('/package/path/top-level.js'), contents: 'MODIFIED TOP LEVEL'},
{path: _('/package/path/folder-1/file-1.js'), contents: 'MODIFIED FILE 1'},
Expand All @@ -47,7 +50,8 @@ runInEachFileSystem(() => {

it('should create backups of all files that previously existed', () => {
const fs = getFileSystem();
const fileWriter = new InPlaceFileWriter(fs);
const logger = new MockLogger();
const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ true);
fileWriter.writeBundle({} as EntryPointBundle, [
{path: _('/package/path/top-level.js'), contents: 'MODIFIED TOP LEVEL'},
{path: _('/package/path/folder-1/file-1.js'), contents: 'MODIFIED FILE 1'},
Expand All @@ -65,15 +69,36 @@ runInEachFileSystem(() => {
expect(fs.exists(_('/package/path/folder-3/file-5.js.__ivy_ngcc_bak'))).toBe(false);
});

it('should error if the backup file already exists', () => {
const fs = getFileSystem();
const fileWriter = new InPlaceFileWriter(fs);
const absoluteBackupPath = _('/package/path/already-backed-up.js');
expect(
() => fileWriter.writeBundle(
{} as EntryPointBundle, [{path: absoluteBackupPath, contents: 'MODIFIED BACKED UP'}]))
.toThrowError(
`Tried to overwrite ${absoluteBackupPath}.__ivy_ngcc_bak with an ngcc back up file, which is disallowed.`);
});
it('should throw an error if the backup file already exists and errorOnFailedEntryPoint is true',
() => {
const fs = getFileSystem();
const logger = new MockLogger();
const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ true);
const absoluteBackupPath = _('/package/path/already-backed-up.js');
expect(
() => fileWriter.writeBundle(
{} as EntryPointBundle,
[{path: absoluteBackupPath, contents: 'MODIFIED BACKED UP'}]))
.toThrowError(
`Tried to overwrite ${absoluteBackupPath}.__ivy_ngcc_bak with an ngcc back up file, which is disallowed.`);
});

it('should log an error, and skip writing the file, if the backup file already exists and errorOnFailedEntryPoint is false',
() => {
const fs = getFileSystem();
const logger = new MockLogger();
const fileWriter = new InPlaceFileWriter(fs, logger, /* errorOnFailedEntryPoint */ false);
const absoluteBackupPath = _('/package/path/already-backed-up.js');
fileWriter.writeBundle(
{} as EntryPointBundle, [{path: absoluteBackupPath, contents: 'MODIFIED BACKED UP'}]);
// Should not have written the new file nor overwritten the backup file.
expect(fs.readFile(absoluteBackupPath)).toEqual('ORIGINAL ALREADY BACKED UP');
expect(fs.readFile(_(absoluteBackupPath + '.__ivy_ngcc_bak'))).toEqual('BACKED UP');
expect(logger.logs.error).toEqual([[
`Tried to write ${absoluteBackupPath}.__ivy_ngcc_bak with an ngcc back up file but it already exists so not writing, nor backing up, ${absoluteBackupPath}.\n` +
`This error may be because two or more entry-points overlap and ngcc has been asked to process some files more than once.\n` +
`You should check other entry-points in this package and set up a config to ignore any that you are not using.`
]]);
});
});
});
19 changes: 11 additions & 8 deletions packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts
View file
Expand Up @@ -23,12 +23,15 @@ runInEachFileSystem(() => {
let _: typeof absoluteFrom;
let fs: FileSystem;
let fileWriter: FileWriter;
let logger: MockLogger;
let entryPoint: EntryPoint;
let esm5bundle: EntryPointBundle;
let esm2015bundle: EntryPointBundle;

beforeEach(() => {
_ = absoluteFrom;
fs = getFileSystem();
logger = new MockLogger();
loadTestFiles([

{
Expand Down Expand Up @@ -100,11 +103,11 @@ runInEachFileSystem(() => {

describe('writeBundle() [primary entry-point]', () => {
beforeEach(() => {
fs = getFileSystem();
fileWriter = new NewEntryPointFileWriter(fs, new DirectPackageJsonUpdater(fs));
fileWriter = new NewEntryPointFileWriter(
fs, logger, /* errorOnFailedEntryPoint */ true, new DirectPackageJsonUpdater(fs));
const config = new NgccConfiguration(fs, _('/'));
const result = getEntryPointInfo(
fs, config, new MockLogger(), _('/node_modules/test'), _('/node_modules/test')) !;
fs, config, logger, _('/node_modules/test'), _('/node_modules/test')) !;
if (result === NO_ENTRY_POINT || result === INVALID_ENTRY_POINT) {
return fail(`Expected an entry point but got ${result}`);
}
Expand Down Expand Up @@ -240,11 +243,11 @@ runInEachFileSystem(() => {

describe('writeBundle() [secondary entry-point]', () => {
beforeEach(() => {
fs = getFileSystem();
fileWriter = new NewEntryPointFileWriter(fs, new DirectPackageJsonUpdater(fs));
fileWriter = new NewEntryPointFileWriter(
fs, logger, /* errorOnFailedEntryPoint */ true, new DirectPackageJsonUpdater(fs));
const config = new NgccConfiguration(fs, _('/'));
const result = getEntryPointInfo(
fs, config, new MockLogger(), _('/node_modules/test'), _('/node_modules/test/a')) !;
fs, config, logger, _('/node_modules/test'), _('/node_modules/test/a')) !;
if (result === NO_ENTRY_POINT || result === INVALID_ENTRY_POINT) {
return fail(`Expected an entry point but got ${result}`);
}
Expand Down Expand Up @@ -369,8 +372,8 @@ runInEachFileSystem(() => {

describe('writeBundle() [entry-point (with files placed outside entry-point folder)]', () => {
beforeEach(() => {
fs = getFileSystem();
fileWriter = new NewEntryPointFileWriter(fs, new DirectPackageJsonUpdater(fs));
fileWriter = new NewEntryPointFileWriter(
fs, logger, /* errorOnFailedEntryPoint */ true, new DirectPackageJsonUpdater(fs));
const config = new NgccConfiguration(fs, _('/'));
const result = getEntryPointInfo(
fs, config, new MockLogger(), _('/node_modules/test'), _('/node_modules/test/b')) !;
Expand Down

0 comments on commit c9f554c

Please sign in to comment.