perf(compiler-cli): ignore the module.id anti-pattern for NgModule ids (

#45024)

In early versions of Angular, it was sometimes necessary to provide a
`moduleId` to `@Component` metadata, and the common pattern for doing this
was to set `moduleId: module.id`. This relied on the bundler to fill in a
value for `module.id`.

However, due to the superficial similarity between `Component.moduleId` and
`NgModule.id`, many users ended up setting `id: module.id` in their
NgModules. This is an anti-pattern that has a few negative effects,
including preventing the NgModule from tree-shaking properly.

This commit changes the compiler to ignore `id: module.id` in NgModules, and
instead provide a warning which suggests removing the line entirely.

PR Close #45024

aio/content/errors/NG6100.md

@@ -0,0 +1,26 @@
+@name NgModule.id Set to module.id anti-pattern
+@category compiler
+@shortDescription Setting NgModule.id to module.id is a common anti-pattern
+
+@description
+Using `module.id` as an NgModule `id` is a common anti-pattern and is likely not serving a useful purpose in your code.
+
+NgModules can be declared with an `id`:
+
+```typescript
+@NgModule({
+  id: 'my_module'
+})
+export class MyModule {}
+```
+
+Declaring an `id` makes the NgModule available for lookup via the `getNgModuleById()` operation. This functionality is rarely used, mainly in very specific bundling scenarios when lazily loading NgModules without obtaining direct references to them. In most Angular code, ES dynamic `import()` (`import('./path/to/module')`) should be used instead, as this provides a direct reference to the NgModule being loaded without the need for a global registration side-effect.
+
+If you are not using `getNgModuleById`, you do not need to provide `id`s for your NgModules. Providing one has a significant drawback: it makes the NgModule non-tree-shakable, which can have an impact on your bundle size.
+
+In particular, the pattern of specifying `id: module.id` results from a misunderstanding of `@NgModule.id`. In earlier versions of Angular, it was sometimes necessary to include the property `moduleId: module.id` in `@Component` metadata.
+
+Using `module.id` for `@NgModule.id` likely results from confusion between `@Component.moduleId` and `@NgModule.id`. `module.id` would not typically be useful for `getNgModuleById()` operations as the `id` needs to be a well-known string, and `module.id` is usually opaque to consumers.
+
+@debugging
+You can remove the `id: module.id` declaration from your NgModules. The compiler ignores this declaration and issues this warning instead.

goldens/public-api/compiler-cli/error_code.md

@@ -77,6 +77,7 @@ export enum ErrorCode {
     VALUE_HAS_WRONG_TYPE = 1010,
     // (undocumented)
     VALUE_NOT_LITERAL = 1011,
+    WARN_NGMODULE_ID_UNNECESSARY = 6100,
     WRITE_TO_READ_ONLY_VARIABLE = 8005
 }
 

packages/compiler-cli/src/ngtsc/annotations/ng_module/src/handler.ts

@@ -265,8 +265,20 @@ export class NgModuleDecoratorHandler implements
       }
     }
 
-    const id: Expression|null =
-        ngModule.has('id') ? new WrappedNodeExpr(ngModule.get('id')!) : null;
+    let id: Expression|null = null;
+    if (ngModule.has('id')) {
+      const idExpr = ngModule.get('id')!;
+      if (!isModuleIdExpression(idExpr)) {
+        id = new WrappedNodeExpr(idExpr);
+      } else {
+        const diag = makeDiagnostic(
+            ErrorCode.WARN_NGMODULE_ID_UNNECESSARY, idExpr,
+            `Using 'module.id' for NgModule.id is a common anti-pattern that is ignored by the Angular compiler.`);
+        diag.category = ts.DiagnosticCategory.Warning;
+        diagnostics.push(diag);
+      }
+    }
+
     const valueContext = node.getSourceFile();
 
     let typeContext = valueContext;
@@ -349,6 +361,7 @@ export class NgModuleDecoratorHandler implements
     };
 
     return {
+      diagnostics: diagnostics.length > 0 ? diagnostics : undefined,
       analysis: {
         id,
         schemas,
@@ -739,3 +752,11 @@ function isNgModule(node: ClassDeclaration, compilation: ScopeData): boolean {
   return !compilation.directives.some(directive => directive.ref.node === node) &&
       !compilation.pipes.some(pipe => pipe.ref.node === node);
 }
+
+/**
+ * Checks whether the given `ts.Expression` is the expression `module.id`.
+ */
+function isModuleIdExpression(expr: ts.Expression): boolean {
+  return ts.isPropertyAccessExpression(expr) && ts.isIdentifier(expr.expression) &&
+      expr.expression.text === 'module' && expr.name.text === 'id';
+}

packages/compiler-cli/src/ngtsc/diagnostics/src/docs.ts

@@ -20,4 +20,5 @@ export const COMPILER_ERRORS_WITH_GUIDES = new Set([
   ErrorCode.SCHEMA_INVALID_ATTRIBUTE,
   ErrorCode.MISSING_REFERENCE_TARGET,
   ErrorCode.COMPONENT_INVALID_SHADOW_DOM_SELECTOR,
+  ErrorCode.WARN_NGMODULE_ID_UNNECESSARY,
 ]);

packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts

@@ -145,6 +145,13 @@ export enum ErrorCode {
    */
   NGMODULE_DECLARATION_IS_STANDALONE = 6008,
 
+  /**
+   * Indicates that an NgModule is declared with `id: module.id`. This is an anti-pattern that is
+   * disabled explicitly in the compiler, that was originally based on a misunderstanding of
+   * `NgModule.id`.
+   */
+  WARN_NGMODULE_ID_UNNECESSARY = 6100,
+
   /**
    * Not actually raised by the compiler, but reserved for documentation of a View Engine error when
    * a View Engine build depends on an Ivy-compiled NgModule.

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

@@ -1233,7 +1233,7 @@ function allTests(os: string) {
       expect(jsContents).toContain(`i0.ɵɵdefineNgModule({ type: TestModule, id: 'test' })`);
     });
 
-    it('should emit the id when the module\'s id is defined as `module.id`', () => {
+    it('should warn when an NgModule id is defined as module.id, and not emit it', () => {
       env.write('index.d.ts', `
          declare const module = {id: string};
        `);
@@ -1244,10 +1244,16 @@ function allTests(os: string) {
          export class TestModule {}
        `);
 
-      env.driveMain();
+      const diags = env.driveDiagnostics();
 
       const jsContents = env.getContents('test.js');
-      expect(jsContents).toContain('i0.ɵɵdefineNgModule({ type: TestModule, id: module.id })');
+      expect(jsContents).toContain('i0.ɵɵdefineNgModule({ type: TestModule })');
+      expect(jsContents).not.toContain('i0.ɵɵregisterNgModuleType');
+
+      expect(diags.length).toEqual(1);
+      expect(diags[0].category).toEqual(ts.DiagnosticCategory.Warning);
+      expect(diags[0].code).toEqual(ngErrorCode(ErrorCode.WARN_NGMODULE_ID_UNNECESSARY));
+      expect(getDiagnosticSourceCode(diags[0])).toEqual('module.id');
     });
 
     it('should emit a side-effectful registration call when an @NgModule has an id', () => {