fix(@angular-devkit/build-angular): several fixes for commonjs warnings

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<string>(['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];
+  }
+
 }

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();
+  });
 });