From 284af7308b1e52efe5ef445ae8c3d6231e9dd633 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Thu, 25 Feb 2021 11:36:17 +0200 Subject: [PATCH] fix(ngcc): do not fail hard when a format-path points to a non-existing or empty file (#40985) Previously, when `ngcc` encountered an entry-point with a format-path that pointed to a non-existing or empty file it would throw an error and stop processing the remaining tasks. In the past, we used to ignore such format-paths and continue processing the rest of the tasks ([see code][1]). This was changed to a hard failure in 2954d1b5ca212548c5c4634a30ce773cfe3b9410. Looking at the code history, the reason for changing the behavior was an (incorrect) assumption that the condition could not fail. This assumption failed to take into account the case where a 3rd-party library has an invalid format-path in its `package.json`. This is an issue with the library, but it should not prevent `ngcc` from processing other packages/entry-points/formats. This commit fixes this by reporting the task as failed but not throwing an error, thus allowing `ngcc` to continue processing other tasks. [1]: https://github.com/angular/angular/blob/3077c9a1f89c5bd75fb96c16e/packages/compiler-cli/ngcc/src/main.ts#L124 Fixes #40965 PR Close #40985 --- .../src/execution/create_compile_function.ts | 11 +-- .../ngcc/src/execution/tasks/completion.ts | 4 +- .../ngcc/test/integration/ngcc_spec.ts | 85 ++++++++++++++++++- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/execution/create_compile_function.ts b/packages/compiler-cli/ngcc/src/execution/create_compile_function.ts index 6f8d754ed2..3b2005d60c 100644 --- a/packages/compiler-cli/ngcc/src/execution/create_compile_function.ts +++ b/packages/compiler-cli/ngcc/src/execution/create_compile_function.ts @@ -46,12 +46,13 @@ export function getCreateCompileFn( // (i.e. they are defined in `entryPoint.packageJson`). Furthermore, they are also guaranteed // to be among `SUPPORTED_FORMAT_PROPERTIES`. // Based on the above, `formatPath` should always be defined and `getEntryPointFormat()` - // should always return a format here (and not `undefined`). + // should always return a format here (and not `undefined`) unless `formatPath` points to a + // missing or empty file. if (!formatPath || !format) { - // This should never happen. - throw new Error( - `Invariant violated: No format-path or format for ${entryPoint.path} : ` + - `${formatProperty} (formatPath: ${formatPath} | format: ${format})`); + onTaskCompleted( + task, TaskProcessingOutcome.Failed, + `property \`${formatProperty}\` pointing to a missing or empty file: ${formatPath}`); + return; } logger.info(`Compiling ${entryPoint.name} : ${formatProperty} as ${format}`); diff --git a/packages/compiler-cli/ngcc/src/execution/tasks/completion.ts b/packages/compiler-cli/ngcc/src/execution/tasks/completion.ts index b984561b28..0fa0081084 100644 --- a/packages/compiler-cli/ngcc/src/execution/tasks/completion.ts +++ b/packages/compiler-cli/ngcc/src/execution/tasks/completion.ts @@ -82,8 +82,8 @@ export function createLogErrorHandler( } function createErrorMessage(fs: ReadonlyFileSystem, task: Task, message: string|null): string { - const jsFormat = - `${task.formatProperty} as ${getEntryPointFormat(fs, task.entryPoint, task.formatProperty)}`; + const jsFormat = `\`${task.formatProperty}\` as ${ + getEntryPointFormat(fs, task.entryPoint, task.formatProperty) ?? 'unknown format'}`; const format = task.typingsOnly ? `typings only using ${jsFormat}` : jsFormat; message = message !== null ? ` due to ${message}` : ''; return `Failed to compile entry-point ${task.entryPoint.name} (${format})` + message; diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 7036b11f1d..657e70592b 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -154,6 +154,85 @@ runInEachFileSystem(() => { expect(loadPackage('local-package', _('/dist')).__processed_by_ivy_ngcc__).toBeUndefined(); }); + it('should report an error, if one of the format-paths is missing or empty', () => { + loadTestFiles([ + // A package with a format-path (main) that points to a missing file. + { + name: _(`/dist/pkg-with-missing-main/package.json`), + contents: ` + { + "name": "pkg-with-missing-main", + "typings": "./index.d.ts", + "es2015": "./index-es2015.js", + "fesm5": "./index-es5.js", + "main": "./index-missing.js" + } + `, + }, + { + name: _('/dist/pkg-with-missing-main/index.d.ts'), + contents: 'export type DummyData = boolean;' + }, + { + name: _('/dist/pkg-with-missing-main/index-es2015.js'), + contents: 'var DUMMY_DATA = true;' + }, + {name: _('/dist/pkg-with-missing-main/index-es5.js'), contents: 'var DUMMY_DATA = true;'}, + {name: _('/dist/pkg-with-missing-main/index.metadata.json'), contents: 'DUMMY DATA'}, + + // A package with a format-path (main) that points to an empty file. + { + name: _(`/dist/pkg-with-empty-main/package.json`), + contents: ` + { + "name": "pkg-with-empty-main", + "typings": "./index.d.ts", + "es2015": "./index-es2015.js", + "fesm5": "./index-es5.js", + "main": "./index-empty.js" + } + `, + }, + { + name: _('/dist/pkg-with-empty-main/index.d.ts'), + contents: 'export type DummyData = boolean;' + }, + {name: _('/dist/pkg-with-empty-main/index-empty.js'), contents: ''}, + {name: _('/dist/pkg-with-empty-main/index-es2015.js'), contents: 'var DUMMY_DATA = true;'}, + {name: _('/dist/pkg-with-empty-main/index-es5.js'), contents: 'var DUMMY_DATA = true;'}, + {name: _('/dist/pkg-with-empty-main/index.metadata.json'), contents: 'DUMMY DATA'}, + ]); + + const logger = new MockLogger(); + mainNgcc({ + basePath: '/dist', + propertiesToConsider: ['es2015', 'main', 'fesm5'], + logger, + }); + + expect(loadPackage('pkg-with-missing-main', _('/dist')).__processed_by_ivy_ngcc__).toEqual({ + es2015: jasmine.any(String), + fesm5: jasmine.any(String), + typings: jasmine.any(String), + }); + expect(loadPackage('pkg-with-empty-main', _('/dist')).__processed_by_ivy_ngcc__).toEqual({ + es2015: jasmine.any(String), + fesm5: jasmine.any(String), + typings: jasmine.any(String), + }); + + expect(logger.logs.error).toEqual([ + [ + 'Failed to compile entry-point pkg-with-missing-main (`main` as unknown format) due to ' + + 'property `main` pointing to a missing or empty file: ./index-missing.js', + ], + [ + 'Failed to compile entry-point pkg-with-empty-main (`main` as unknown format) due to ' + + 'property `main` pointing to a missing or empty file: ./index-empty.js', + ], + ]); + }); + it('should generate correct metadata for decorated getter/setter properties', () => { setupAngularCoreEsm5(); compileIntoFlatEs5Package('test-package', { @@ -573,7 +652,7 @@ runInEachFileSystem(() => { fail('should have thrown'); } catch (e) { expect(e.message).toContain( - 'Failed to compile entry-point test-package (esm2015 as esm2015) due to compilation errors:'); + 'Failed to compile entry-point test-package (`esm2015` as esm2015) due to compilation errors:'); expect(e.message).toContain('NG1010'); expect(e.message).toContain('selector must be a string'); } @@ -1552,7 +1631,7 @@ runInEachFileSystem(() => { fail('should have thrown'); } catch (e) { expect(e.message).toContain( - 'Failed to compile entry-point fatal-error (es2015 as esm2015) due to compilation errors:'); + 'Failed to compile entry-point fatal-error (`es2015` as esm2015) due to compilation errors:'); expect(e.message).toContain('NG2001'); expect(e.message).toContain('component is missing a template'); } @@ -1630,7 +1709,7 @@ runInEachFileSystem(() => { expect(logger.logs.error.length).toEqual(1); const message = logger.logs.error[0][0]; expect(message).toContain( - 'Failed to compile entry-point fatal-error (es2015 as esm2015) due to compilation errors:'); + 'Failed to compile entry-point fatal-error (`es2015` as esm2015) due to compilation errors:'); expect(message).toContain('NG2001'); expect(message).toContain('component is missing a template');