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 index 656f9f01d74f..7b29299322cd 100644 --- 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 @@ -34,14 +34,10 @@ export class CommonJsUsageWarnPlugin { // Allow the below depedency for HMR // tslint:disable-next-line: max-line-length // https://github.com/angular/angular-cli/blob/1e258317b1f6ec1e957ee3559cc3b28ba602f3ba/packages/angular_devkit/build_angular/src/dev-server/index.ts#L605-L638 - private allowedDepedencies = [ - 'webpack/hot/dev-server', - ]; + private allowedDepedencies = new Set(['webpack/hot/dev-server']); constructor(private options: CommonJsUsageWarnPluginOptions = {}) { - if (this.options.allowedDepedencies) { - this.allowedDepedencies.push(...this.options.allowedDepedencies); - } + this.options.allowedDepedencies?.forEach(d => this.allowedDepedencies.add(d)); } apply(compiler: Compiler) { @@ -57,7 +53,10 @@ export class CommonJsUsageWarnPlugin { continue; } - if (this.allowedDepedencies?.includes(rawRequest)) { + if ( + this.allowedDepedencies.has(rawRequest) || + this.allowedDepedencies.has(this.rawRequestToPackageName(rawRequest)) + ) { // Skip as this module is allowed even if it's a CommonJS. continue; } @@ -67,7 +66,8 @@ export class CommonJsUsageWarnPlugin { // 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 ?? [])) { + const parentDependencies = issuer?.issuer?.dependencies; + if (parentDependencies && this.hasCommonJsDependencies(parentDependencies)) { continue; } @@ -81,8 +81,16 @@ export class CommonJsUsageWarnPlugin { // And if the issuer request is not from 'webpack-dev-server', as 'webpack-dev-server' // will require CommonJS libraries for live reloading such as 'sockjs-node'. if (mainIssuer?.name === 'main' && !issuer?.userRequest?.includes('webpack-dev-server')) { - const warning = `${issuer?.userRequest} depends on ${rawRequest}. CommonJS or AMD dependencies can cause optimization bailouts.\n` + - 'For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies'; + let warning = `${issuer?.userRequest} depends on '${rawRequest}'.`; + + if (rawRequest.startsWith('@angular/common/locales')) { + warning += `\nWhen using the 'localize' option this import is not needed. ` + + `Did you mean to import '${rawRequest.replace(/locales(\/extra)?\//, 'locales/global/')}'?\n` + + 'For more info see: https://angular.io/guide/i18n#import-global-variants-of-the-locale-data'; + } else { + warning += ' CommonJS or AMD dependencies can cause optimization bailouts.\n' + + 'For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies'; + } // Avoid showing the same warning multiple times when in 'watch' mode. if (!this.shownWarnings.has(warning)) { @@ -100,4 +108,12 @@ export class CommonJsUsageWarnPlugin { return dependencies.some(d => d instanceof CommonJsRequireDependency || d instanceof AMDDefineDependency); } + private rawRequestToPackageName(rawRequest: string): string { + return rawRequest.startsWith('@') + // Scoped request ex: @angular/common/locale/en -> @angular/common + ? rawRequest.split('/', 2).join('/') + // Non-scoped request ex: lodash/isEmpty -> lodash + : rawRequest.split('/', 1)[0]; + } + } diff --git a/packages/angular_devkit/build_angular/src/browser/specs/common-js-warning_spec.ts b/packages/angular_devkit/build_angular/src/browser/specs/common-js-warning_spec.ts index aaa11a09a9ee..da10d226a481 100644 --- a/packages/angular_devkit/build_angular/src/browser/specs/common-js-warning_spec.ts +++ b/packages/angular_devkit/build_angular/src/browser/specs/common-js-warning_spec.ts @@ -20,9 +20,6 @@ describe('Browser Builder commonjs warning', () => { 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 = []; @@ -32,19 +29,31 @@ describe('Browser Builder commonjs warning', () => { afterEach(async () => host.restore().toPromise()); it('should show warning when depending on a Common JS bundle', async () => { + // Add a Common JS dependency + host.appendToFile('src/app/app.component.ts', ` + import 'bootstrap'; + `); + 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).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 () => { + // Add a Common JS dependency + host.appendToFile('src/app/app.component.ts', ` + import 'bootstrap'; + import 'zone.js/dist/zone-error'; + `); + const overrides = { allowedCommonJsDependencies: [ 'bootstrap', + 'zone.js', ], }; @@ -54,4 +63,20 @@ describe('Browser Builder commonjs warning', () => { expect(logs.join()).not.toContain('WARNING'); await run.stop(); }); + + it(`should show warning when importing non global '@angular/common/locale' data`, async () => { + // Add a Common JS dependency + host.appendToFile('src/app/app.component.ts', ` + import '@angular/common/locales/fr'; + `); + + 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 '@angular\/common\/locales\/fr'/); + expect(logMsg).toContain(`Did you mean to import '@angular/common/locales/global/fr'`); + await run.stop(); + }); });