refactor(compiler-cli): restrict read option values for signal-based queries (angular#54257)

devversion · thePunderWoman · commit f8c02b69b934 · 2024-02-06T16:04:10.000Z
The `read` option for queries can rely on lexical variables inside the class. These constructs are fine from a technical perspective in TypeScript, but in practice, when the component/directive definition is being created, the read value is extracted into the definition, **outside** of the class. This breaks `this` references. To fix this, we are restricting the `read` option to literal values. Similar to `descendants`. Literal references are in practice constructs like: - `read: bla.X` - `read: X` where `bla` or `X` is never a `ThisKeywoord`- hence fixing the issue and also simplifying the patterns for easier single file compilation. PR Close angular#54257
diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/query_functions.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/query_functions.ts
@@ -94,9 +94,26 @@ function parseLocator(expression: ts.Expression, reflector: ReflectionHost): str
       unwrappedExpression !== null ? ForwardRefHandling.Unwrapped : ForwardRefHandling.None);
 }
 
-/** Parses the `read` option of a query. */
+/**
+ * Parses the `read` option of a query.
+ *
+ * We only support the following patterns for the `read` option:
+ *     - `read: someImport.BLA`,
+ *     - `read: BLA`
+ *
+ * That is because we cannot trivially support complex expressions,
+ * especially those referencing `this`. The read provider token will
+ * live outside of the class in the static class definition.
+ */
 function parseReadOption(value: ts.Expression): o.Expression {
-  return new o.WrappedNodeExpr(value);
+  if (ts.isPropertyAccessExpression(value) && ts.isIdentifier(value.expression) ||
+      ts.isIdentifier(value)) {
+    return new o.WrappedNodeExpr(value);
+  }
+
+  throw new FatalDiagnosticError(
+      ErrorCode.VALUE_NOT_LITERAL, value,
+      `Query "read" option expected a literal class reference.`);
 }
 
 /** Parses the `descendants` option of a query. */
diff --git a/packages/compiler-cli/test/ngtsc/authoring_queries_spec.ts b/packages/compiler-cli/test/ngtsc/authoring_queries_spec.ts
@@ -38,6 +38,28 @@ runInEachFileSystem(() => {
       expect(js).toContain(`i0.&#629;&#629;queryAdvance();`);
     });
 
+    it('should support viewChild with `read` options', () => {
+      env.write('other-file.ts', `export class X {}`);
+      env.write('test.ts', `
+        import {Component, viewChild} from '@angular/core';
+        import * as fromOtherFile from './other-file';
+
+        class X {}
+
+        @Component({selector: 'test', template: ''})
+        export class TestDir {
+          el = viewChild('myLocator', {read: X});
+          el2 = viewChild('myLocator', {read: fromOtherFile.X});
+        }
+      `);
+      env.driveMain();
+
+      const js = env.getContents('test.js');
+      expect(js).toContain(`i0.&#629;&#629;viewQuerySignal(ctx.el, _c0, 5, X);`);
+      expect(js).toContain(`i0.&#629;&#629;viewQuerySignal(ctx.el2, _c0, 5, fromOtherFile.X);`);
+      expect(js).toContain(`i0.&#629;&#629;queryAdvance(2);`);
+    });
+
     it('should handle a basic viewChildren', () => {
       env.write('test.ts', `
         import {Component, viewChildren} from '@angular/core';
@@ -184,6 +206,25 @@ runInEachFileSystem(() => {
           messageText: `Decorator query metadata must be an instance of a query type`,
         })]);
       });
+
+      it('should report an error when `read` option is complex', () => {
+        env.write('test.ts', `
+        import {Directive, ViewChild, viewChild} from '@angular/core';
+
+        @Directive({
+          selector: 'test',
+        })
+        export class TestDir {
+          something = null!;
+          el = viewChild('myLocator', {read: this.something});
+        }
+      `);
+        const diagnostics = env.driveDiagnostics();
+        expect(diagnostics.length).toBe(1);
+        expect(diagnostics).toEqual([jasmine.objectContaining({
+          messageText: `Query "read" option expected a literal class reference.`,
+        })]);
+      });
     });
   });
 });
