From 7db269ba6a98bfadced9b341111461016e03d26a Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 5 Aug 2019 13:36:51 +0300 Subject: [PATCH] fix(ivy): ngcc - correctly detect formats processed in previous runs (#32003) Previously, `ngcc` would avoid processing a `formatPath` that a property in `package.json` mapped to, if either the _property_ was marked as processed or the `formatPath` (i.e. the file(s)) was processed in the same `ngcc` run (since the `compiledFormats` set was not persisted across runs). This could lead in a situation where a `formatPath` would be compiled twice (if for example properties `a` and `b` both mapped to the same `formatPath` and one would run `ngcc` for property `a` and then `b`). This commit fixes it by ensuring that as soon as a `formatPath` has been processed all corresponding properties are marked as processed (which persists across `ngcc` runs). PR Close #32003 --- packages/compiler-cli/ngcc/src/main.ts | 45 +++++++++++------ .../ngcc/test/integration/ngcc_spec.ts | 50 +++++++++++++++++++ 2 files changed, 79 insertions(+), 16 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/main.ts b/packages/compiler-cli/ngcc/src/main.ts index 9b4af20222..544b265c4d 100644 --- a/packages/compiler-cli/ngcc/src/main.ts +++ b/packages/compiler-cli/ngcc/src/main.ts @@ -18,7 +18,7 @@ import {ConsoleLogger, LogLevel} from './logging/console_logger'; import {Logger} from './logging/logger'; import {hasBeenProcessed, markAsProcessed} from './packages/build_marker'; import {NgccConfiguration} from './packages/configuration'; -import {EntryPoint, EntryPointJsonProperty, SUPPORTED_FORMAT_PROPERTIES, getEntryPointFormat} from './packages/entry_point'; +import {EntryPoint, EntryPointJsonProperty, EntryPointPackageJson, SUPPORTED_FORMAT_PROPERTIES, getEntryPointFormat} from './packages/entry_point'; import {makeEntryPointBundle} from './packages/entry_point_bundle'; import {Transformer} from './packages/transformer'; import {PathMappings} from './utils'; @@ -107,6 +107,7 @@ export function mainNgcc( const compiledFormats = new Set(); const entryPointPackageJson = entryPoint.packageJson; const entryPointPackageJsonPath = fileSystem.resolve(entryPoint.path, 'package.json'); + const pathToPropsMap = getFormatPathToPropertiesMap(entryPointPackageJson); let processDts = !hasBeenProcessed(entryPointPackageJson, 'typings'); @@ -128,35 +129,30 @@ export function mainNgcc( } - // We don't break if this if statement fails because we still want to mark - // the property as processed even if its underlying format has been built already. if (!compiledFormats.has(formatPath)) { const bundle = makeEntryPointBundle( fileSystem, entryPoint, formatPath, isCore, property, format, processDts, pathMappings, true); + if (bundle) { logger.info(`Compiling ${entryPoint.name} : ${property} as ${format}`); const transformedFiles = transformer.transform(bundle); fileWriter.writeBundle(entryPoint, bundle, transformedFiles); compiledFormats.add(formatPath); + + const propsToMarkAsProcessed = pathToPropsMap.get(formatPath) !; + if (processDts) { + propsToMarkAsProcessed.push('typings'); + processDts = false; + } + + markAsProcessed( + fileSystem, entryPointPackageJson, entryPointPackageJsonPath, propsToMarkAsProcessed); } else { logger.warn( `Skipping ${entryPoint.name} : ${format} (no valid entry point file for this format).`); } } - - // Either this format was just compiled or its underlying format was compiled because of a - // previous property. - if (compiledFormats.has(formatPath)) { - const propsToMarkAsProcessed = [property]; - if (processDts) { - propsToMarkAsProcessed.push('typings'); - processDts = false; - } - - markAsProcessed( - fileSystem, entryPointPackageJson, entryPointPackageJsonPath, propsToMarkAsProcessed); - } } if (compiledFormats.size === 0) { @@ -264,3 +260,20 @@ function logInvalidEntryPoints(logger: Logger, invalidEntryPoints: InvalidEntryP invalidEntryPoint.missingDependencies.map(dep => ` - ${dep}`).join('\n')); }); } + +function getFormatPathToPropertiesMap(packageJson: EntryPointPackageJson): + Map { + const map = new Map(); + + for (const prop of SUPPORTED_FORMAT_PROPERTIES) { + const formatPath = packageJson[prop]; + if (formatPath) { + if (!map.has(formatPath)) { + map.set(formatPath, []); + } + map.get(formatPath) !.push(prop); + } + } + + return map; +} diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index dee7ca76ff..6357078a1b 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -163,6 +163,23 @@ runInEachFileSystem(() => { ]); }); }); + + it('should skip all processing if the first matching `propertyToConsider` is marked as processed', + () => { + const logger = new MockLogger(); + markPropertiesAsProcessed('@angular/common/http/testing', ['esm2015']); + mainNgcc({ + basePath: '/node_modules', + targetEntryPointPath: '@angular/common/http/testing', + // Simulate a property that does not exist on the package.json and will be ignored. + propertiesToConsider: ['missing', 'esm2015', 'esm5'], + compileAllFormats: false, logger, + }); + + expect(logger.logs.debug).toContain([ + 'The target entry-point has already been processed' + ]); + }); }); @@ -214,6 +231,33 @@ runInEachFileSystem(() => { typings: '0.0.0-PLACEHOLDER', }); }); + + it('should mark all matching properties as processed in order not to compile them on a subsequent run', + () => { + const logger = new MockLogger(); + const logs = logger.logs.debug; + + // `fesm2015` and `es2015` map to the same file: `./fesm2015/common.js` + mainNgcc({ + basePath: '/node_modules/@angular/common', + propertiesToConsider: ['fesm2015'], logger, + }); + + expect(logs).not.toContain(['Skipping @angular/common : es2015 (already compiled).']); + expect(loadPackage('@angular/common').__processed_by_ivy_ngcc__).toEqual({ + es2015: '0.0.0-PLACEHOLDER', + fesm2015: '0.0.0-PLACEHOLDER', + typings: '0.0.0-PLACEHOLDER', + }); + + // Now, compiling `es2015` should be a no-op. + mainNgcc({ + basePath: '/node_modules/@angular/common', + propertiesToConsider: ['es2015'], logger, + }); + + expect(logs).toContain(['Skipping @angular/common : es2015 (already compiled).']); + }); }); describe('with compileAllFormats set to false', () => { @@ -260,6 +304,7 @@ runInEachFileSystem(() => { }); expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({ + fesm5: '0.0.0-PLACEHOLDER', module: '0.0.0-PLACEHOLDER', typings: '0.0.0-PLACEHOLDER', }); @@ -272,6 +317,7 @@ runInEachFileSystem(() => { }); expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({ esm5: '0.0.0-PLACEHOLDER', + fesm5: '0.0.0-PLACEHOLDER', module: '0.0.0-PLACEHOLDER', typings: '0.0.0-PLACEHOLDER', }); @@ -346,6 +392,7 @@ runInEachFileSystem(() => { }); expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({ es2015: '0.0.0-PLACEHOLDER', + fesm2015: '0.0.0-PLACEHOLDER', typings: '0.0.0-PLACEHOLDER', }); expect(loadPackage('local-package', _('/dist')).__processed_by_ivy_ngcc__).toEqual({ @@ -403,6 +450,7 @@ runInEachFileSystem(() => { }); expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({ es2015: '0.0.0-PLACEHOLDER', + fesm2015: '0.0.0-PLACEHOLDER', typings: '0.0.0-PLACEHOLDER', }); }); @@ -429,6 +477,7 @@ runInEachFileSystem(() => { // We process core but not core/testing. expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({ es2015: '0.0.0-PLACEHOLDER', + fesm2015: '0.0.0-PLACEHOLDER', typings: '0.0.0-PLACEHOLDER', }); expect(loadPackage('@angular/core/testing').__processed_by_ivy_ngcc__).toBeUndefined(); @@ -436,6 +485,7 @@ runInEachFileSystem(() => { expect(loadPackage('@angular/common').__processed_by_ivy_ngcc__).toBeUndefined(); expect(loadPackage('@angular/common/http').__processed_by_ivy_ngcc__).toEqual({ es2015: '0.0.0-PLACEHOLDER', + fesm2015: '0.0.0-PLACEHOLDER', typings: '0.0.0-PLACEHOLDER', }); });