From ea11c5549ab9127ac02679cfa0a8dc00626e8389 Mon Sep 17 00:00:00 2001 From: Alan Agius <alan.agius4@gmail.com> Date: Mon, 23 Mar 2020 11:51:51 +0100 Subject: [PATCH] feat(@angular-devkit/build-angular): show warnings when depending on CommonJS. Depending on CommonJS modules is know to cause optimization bailouts. With this change when running a browser build and scripts optimization is enabled we display a warning. To suppress the warning for a particular package, users can use the `allowedCommonJsDepedencies` builder options. Example: ``` "build": { "builder": "@angular-devkit/build-angular:browser", "options": { ... "allowedCommonJsDepedencies": ["bootstrap"] }, } ``` Reference: TOOL-1328 --- packages/angular/cli/lib/config/schema.json | 7 ++ .../angular-cli-files/models/build-options.ts | 2 +- .../models/webpack-configs/browser.ts | 12 ++- .../models/webpack-configs/common.ts | 50 +++++----- .../plugins/common-js-usage-warn-plugin.ts | 91 +++++++++++++++++++ .../src/angular-cli-files/plugins/webpack.ts | 3 +- .../build_angular/src/browser/schema.json | 8 ++ .../browser/common-js-warning_spec_large.ts | 57 ++++++++++++ 8 files changed, 202 insertions(+), 28 deletions(-) create mode 100644 packages/angular_devkit/build_angular/src/angular-cli-files/plugins/common-js-usage-warn-plugin.ts create mode 100644 packages/angular_devkit/build_angular/test/browser/common-js-warning_spec_large.ts diff --git a/packages/angular/cli/lib/config/schema.json b/packages/angular/cli/lib/config/schema.json index acb1afd02599..fc37080444a1 100644 --- a/packages/angular/cli/lib/config/schema.json +++ b/packages/angular/cli/lib/config/schema.json @@ -974,6 +974,13 @@ "anonymous", "use-credentials" ] + }, + "allowedCommonJsDependencies": { + "description": "A list of CommonJS packages that are allowed to be used without a built time warning.", + "type": "array", + "items": { + "type": "string" + } } }, "additionalProperties": false, diff --git a/packages/angular_devkit/build_angular/src/angular-cli-files/models/build-options.ts b/packages/angular_devkit/build_angular/src/angular-cli-files/models/build-options.ts index 072fe41f85f5..996a922a6094 100644 --- a/packages/angular_devkit/build_angular/src/angular-cli-files/models/build-options.ts +++ b/packages/angular_devkit/build_angular/src/angular-cli-files/models/build-options.ts @@ -87,8 +87,8 @@ export interface BuildOptions { /* Append script target version to filename. */ esVersionInFileName?: boolean; - experimentalRollupPass?: boolean; + allowedCommonJsDependencies?: string[]; } export interface WebpackTestOptions extends BuildOptions { diff --git a/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/browser.ts b/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/browser.ts index f687f14a9588..e60c28632f97 100644 --- a/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/browser.ts +++ b/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/browser.ts @@ -7,6 +7,7 @@ */ import { LicenseWebpackPlugin } from 'license-webpack-plugin'; import * as webpack from 'webpack'; +import { CommonJsUsageWarnPlugin } from '../../plugins/webpack'; import { WebpackConfigOptions } from '../build-options'; import { getSourceMapDevTool, isPolyfillsEntry, normalizeExtraEntryPoints } from './utils'; @@ -23,12 +24,14 @@ export function getBrowserConfig(wco: WebpackConfigOptions): webpack.Configurati vendorChunk, commonChunk, styles, + allowedCommonJsDependencies, + optimization, } = buildOptions; const extraPlugins = []; let isEval = false; - const { styles: stylesOptimization, scripts: scriptsOptimization } = buildOptions.optimization; + const { styles: stylesOptimization, scripts: scriptsOptimization } = optimization; const { styles: stylesSourceMap, scripts: scriptsSourceMap, @@ -123,7 +126,12 @@ export function getBrowserConfig(wco: WebpackConfigOptions): webpack.Configurati }, }, }, - plugins: extraPlugins, + plugins: [ + new CommonJsUsageWarnPlugin({ + allowedDepedencies: allowedCommonJsDependencies, + }), + ...extraPlugins, + ], node: false, }; } diff --git a/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts b/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts index 109f41c5a232..8601b4e6a91e 100644 --- a/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts +++ b/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/common.ts @@ -36,11 +36,13 @@ import { cachingDisabled, shouldBeautify, } from '../../../utils/environment-options'; -import { BundleBudgetPlugin } from '../../plugins/bundle-budget'; -import { NamedLazyChunksPlugin } from '../../plugins/named-chunks-plugin'; -import { OptimizeCssWebpackPlugin } from '../../plugins/optimize-css-webpack-plugin'; -import { ScriptsWebpackPlugin } from '../../plugins/scripts-webpack-plugin'; -import { WebpackRollupLoader } from '../../plugins/webpack'; +import { + BundleBudgetPlugin, + NamedLazyChunksPlugin, + OptimizeCssWebpackPlugin, + ScriptsWebpackPlugin, + WebpackRollupLoader, +} from '../../plugins/webpack'; import { findAllNodeModules } from '../../utilities/find-up'; import { WebpackConfigOptions } from '../build-options'; import { getEsVersionForFileName, getOutputHashFormat, normalizeExtraEntryPoints } from './utils'; @@ -149,7 +151,7 @@ export function getCommonConfig(wco: WebpackConfigOptions): Configuration { // tslint:disable-next-line: no-any (compilation.mainTemplate.hooks as any).assetPath.tap( 'build-angular', - (filename: string | ((data: ChunkData) => string), data: ChunkData) => { + (filename: string | ((data: ChunkData) => string), data: ChunkData) => { const assetName = typeof filename === 'function' ? filename(data) : filename; const isMap = assetName && assetName.endsWith('.map'); @@ -313,6 +315,12 @@ export function getCommonConfig(wco: WebpackConfigOptions): Configuration { extraPlugins.push(new NamedLazyChunksPlugin()); } + if (!differentialLoadingMode) { + // Budgets are computed after differential builds, not via a plugin. + // https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/browser/index.ts + extraPlugins.push(new BundleBudgetPlugin({ budgets: buildOptions.budgets })); + } + let sourceMapUseRule; if ((scriptsSourceMap || stylesSourceMap) && vendorSourceMap) { sourceMapUseRule = { @@ -411,18 +419,18 @@ export function getCommonConfig(wco: WebpackConfigOptions): Configuration { allowMinify && (buildOptions.platform == 'server' ? { - ecma: terserEcma, - global_defs: angularGlobalDefinitions, - keep_fnames: true, - } + ecma: terserEcma, + global_defs: angularGlobalDefinitions, + keep_fnames: true, + } : { - ecma: terserEcma, - pure_getters: buildOptions.buildOptimizer, - // PURE comments work best with 3 passes. - // See https://github.com/webpack/webpack/issues/2899#issuecomment-317425926. - passes: buildOptions.buildOptimizer ? 3 : 1, - global_defs: angularGlobalDefinitions, - }), + ecma: terserEcma, + pure_getters: buildOptions.buildOptimizer, + // PURE comments work best with 3 passes. + // See https://github.com/webpack/webpack/issues/2899#issuecomment-317425926. + passes: buildOptions.buildOptimizer ? 3 : 1, + global_defs: angularGlobalDefinitions, + }), // We also want to avoid mangling on server. // Name mangling is handled within the browser builder mangle: allowMangle && buildOptions.platform !== 'server' && !differentialLoadingMode, @@ -543,13 +551,7 @@ export function getCommonConfig(wco: WebpackConfigOptions): Configuration { minimizer: [ new HashedModuleIdsPlugin(), ...extraMinimizers, - ].concat(differentialLoadingMode ? [ - // Budgets are computed after differential builds, not via a plugin. - // https://github.com/angular/angular-cli/blob/master/packages/angular_devkit/build_angular/src/browser/index.ts - ] : [ - // Non differential builds should be computed here, as a plugin. - new BundleBudgetPlugin({ budgets: buildOptions.budgets }), - ]), + ], }, plugins: [ // Always replace the context for the System.import in angular/core to prevent warnings. diff --git a/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/common-js-usage-warn-plugin.ts b/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/common-js-usage-warn-plugin.ts new file mode 100644 index 000000000000..ca2a45985bcb --- /dev/null +++ b/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/common-js-usage-warn-plugin.ts @@ -0,0 +1,91 @@ + +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import { isAbsolute } from 'path'; +import { Compiler, compilation } from 'webpack'; + +// Webpack doesn't export these so the deep imports can potentially break. +const CommonJsRequireDependency = require('webpack/lib/dependencies/CommonJsRequireDependency'); +const AMDDefineDependency = require('webpack/lib/dependencies/AMDDefineDependency'); + +// The below is extended because there are not in the typings +interface WebpackModule extends compilation.Module { + name?: string; + rawRequest?: string; + dependencies: unknown[]; + issuer: WebpackModule | null; + userRequest?: string; +} + +export interface CommonJsUsageWarnPluginOptions { + /** A list of CommonJS packages that are allowed to be used without a warning. */ + allowedDepedencies?: string[]; +} + +export class CommonJsUsageWarnPlugin { + private shownWarnings = new Set<string>(); + + constructor(private options: CommonJsUsageWarnPluginOptions = {}) { + + } + + apply(compiler: Compiler) { + compiler.hooks.compilation.tap('CommonJsUsageWarnPlugin', compilation => { + compilation.hooks.finishModules.tap('CommonJsUsageWarnPlugin', modules => { + for (const { dependencies, rawRequest, issuer } of modules as unknown as WebpackModule[]) { + if ( + !rawRequest || + rawRequest.startsWith('.') || + isAbsolute(rawRequest) + ) { + // Skip if module is absolute or relative. + continue; + } + + if (this.options.allowedDepedencies?.includes(rawRequest)) { + // Skip as this module is allowed even if it's a CommonJS. + continue; + } + + if (this.hasCommonJsDependencies(dependencies)) { + // Dependency is CommonsJS or AMD. + + // Check if it's parent issuer is also a CommonJS dependency. + // In case it is skip as an warning will be show for the parent CommonJS dependency. + if (this.hasCommonJsDependencies(issuer?.issuer?.dependencies ?? [])) { + continue; + } + + // Find the main issuer (entry-point). + let mainIssuer = issuer; + while (mainIssuer?.issuer) { + mainIssuer = mainIssuer.issuer; + } + + // Only show warnings for modules from main entrypoint. + if (mainIssuer?.name === 'main') { + const warning = `${issuer?.userRequest} depends on ${rawRequest}. CommonJS or AMD dependencies can cause optimization bailouts.`; + + // Avoid showing the same warning multiple times when in 'watch' mode. + if (!this.shownWarnings.has(warning)) { + compilation.warnings.push(warning); + this.shownWarnings.add(warning); + } + } + } + } + }); + }); + } + + private hasCommonJsDependencies(dependencies: unknown[]): boolean { + return dependencies.some(d => d instanceof CommonJsRequireDependency || d instanceof AMDDefineDependency); + } + +} diff --git a/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/webpack.ts b/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/webpack.ts index d1c1ee4ac82b..4a0b6127ffd9 100644 --- a/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/webpack.ts +++ b/packages/angular_devkit/build_angular/src/angular-cli-files/plugins/webpack.ts @@ -13,7 +13,8 @@ export { BundleBudgetPlugin, BundleBudgetPluginOptions } from './bundle-budget'; export { ScriptsWebpackPlugin, ScriptsWebpackPluginOptions } from './scripts-webpack-plugin'; export { SuppressExtractedTextChunksWebpackPlugin } from './suppress-entry-chunks-webpack-plugin'; export { RemoveHashPlugin, RemoveHashPluginOptions } from './remove-hash-plugin'; -export { NamedLazyChunksPlugin as NamedChunksPlugin } from './named-chunks-plugin'; +export { NamedLazyChunksPlugin } from './named-chunks-plugin'; +export { CommonJsUsageWarnPlugin } from './common-js-usage-warn-plugin'; export { default as PostcssCliResources, PostcssCliResourcesOptions, diff --git a/packages/angular_devkit/build_angular/src/browser/schema.json b/packages/angular_devkit/build_angular/src/browser/schema.json index c26ab38658bd..347c2ed04886 100644 --- a/packages/angular_devkit/build_angular/src/browser/schema.json +++ b/packages/angular_devkit/build_angular/src/browser/schema.json @@ -379,6 +379,14 @@ "type": "boolean", "description": "Concatenate modules with Rollup before bundling them with Webpack.", "default": false + }, + "allowedCommonJsDependencies": { + "description": "A list of CommonJS packages that are allowed to be used without a built time warning.", + "type": "array", + "items": { + "type": "string" + }, + "default": [] } }, "additionalProperties": false, diff --git a/packages/angular_devkit/build_angular/test/browser/common-js-warning_spec_large.ts b/packages/angular_devkit/build_angular/test/browser/common-js-warning_spec_large.ts new file mode 100644 index 000000000000..6ead00dca7c1 --- /dev/null +++ b/packages/angular_devkit/build_angular/test/browser/common-js-warning_spec_large.ts @@ -0,0 +1,57 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import { Architect } from '@angular-devkit/architect'; +import { logging } from '@angular-devkit/core'; +import { createArchitect, host } from '../utils'; + +describe('Browser Builder commonjs warning', () => { + const targetSpec = { project: 'app', target: 'build' }; + + let architect: Architect; + let logger: logging.Logger; + let logs: string[]; + + beforeEach(async () => { + await host.initialize().toPromise(); + architect = (await createArchitect(host.root())).architect; + + // Add a Common JS dependency + host.appendToFile('src/app/app.component.ts', `import 'bootstrap';`); + + // Create logger + logger = new logging.Logger(''); + logs = []; + logger.subscribe(e => logs.push(e.message)); + }); + + afterEach(async () => host.restore().toPromise()); + + it('should show warning when depending on a Common JS bundle', async () => { + const run = await architect.scheduleTarget(targetSpec, undefined, { logger }); + const output = await run.result; + expect(output.success).toBe(true); + const logMsg = logs.join(); + expect(logMsg).toMatch(/WARNING in.+app\.component\.ts depends on bootstrap\. CommonJS or AMD dependencies/); + expect(logMsg).not.toContain('jquery', 'Should not warn on transitive CommonJS packages which parent is also CommonJS.'); + await run.stop(); + }); + + it('should not show warning when depending on a Common JS bundle which is allowed', async () => { + const overrides = { + allowedCommonJsDependencies: [ + 'bootstrap', + ], + }; + + const run = await architect.scheduleTarget(targetSpec, overrides, { logger }); + const output = await run.result; + expect(output.success).toBe(true); + expect(logs.join()).not.toContain('WARNING'); + await run.stop(); + }); +});