Class symbols in ngcc's reflection host

This document serves as context and proposal for resolving FW-1507.

Introduction

Ngcc's reflection host for reflecting ESM2015, ESM5, CommonJS and UMD bundles exposes a ClassSymbol by means of NgccReflectionHost.getClassSymbol. This symbol is a regular ts.Symbol created by TypeScript during its binding phase, that represents a certain class in the JavaScript bundle. Information from this symbol is used for various reflection tasks, such as retrieving all members of a class.

That sounds easy enough, however ngcc's need to deal with downleveled source code introduces a challenge. In downleveled code, a class can appear in a vastly different form compared to its originating TypeScript source. For instance, a class with TypeScript source

@Component()
export class MyClass {}

May be emitted to ESM2015 flavored JavaScript:

let MyClass = class MyClass {};
MyClass.decorators = [...];

export { MyClass };

This may in turn be downleveled to ESM5:

var MyClass = function() {
  function MyClass() {
    
  }
  MyClass.decorators = [...];
  return MyClass;
}();

export { MyClass };

As can be seen from the above examples, the class declaration MyClass has two declarations in the JavaScript code; one outer declaration corresponding with the publicly visible variable, and an inner declaration with the actual class implementation.

The problem

Given the two declaration places for a class, TypeScript's binder will have created two ts.Symbols, one for each declaration. This poses a question: which symbol should be returned from NgccReflectionHost.getClassSymbol?

The answer to this question should be the symbol corresponding with the outer declaration, as that is the one that is publicly exported and therefore visible to the outside world. The inner declaration is simply an implementation detail that is not allowed to "leak" from the reflection host.

Unfortunately however, the real answer is that the returned symbol is actually different for the different reflection host implementations. For ESM2015, it is the inner class declaration, whereas for ESM5 and derivates it is the outer declaration. The reason is that Esm2015ReflectionHost.getClassSymbol is using Esm2015ReflectionHost.getClassDeclaration, which returns the inner declaration. Note that this is okay, as getClassDeclaration itself is not a member of the NgccReflectionHost interface, however getClassSymbol itself is exposed through the interface.

Options

I see a couple options to resolve the discrepancy across the reflection hosts, thereby avoiding the issue.

1. Change Esm2015ReflectionHost.getClassSymbol to return the symbol corresponding with the outer declaration.

The downside of this approach is that most information of interest is actually present on the symbol for the inner declaration, so this approach would require frequent searches for the inner declaration/symbol.

Also, this doesn't really help with avoiding bugs like FW-1507 going forward. Given a ts.Symbol, it is not immediately obvious whether it corresponds with the outer or inner declaration.

2. Change NgccReflectionHost.getClassSymbol to return a type that encompasses both symbols.

Since NgccReflectionHost.getClassSymbol is specific to ngcc already, its return type could acknowledge the existence of two symbols for a class:

export interface NgccClassSymbol {
  name: string;
  outerSymbol: ts.Symbol;
  innerSymbol: ts.Symbol;
}

When there's only one declaration and symbol–-that could occur in ESM2015 code–-I propose that outerSymbol === innerSymbol.

I worked on a draft in https://github.com/JoostK/angular/commits/ngcc-class-symbol, so see how well this would work. All testcases still pass, although a testcase to verify whether FW-1507 has been fixed has not yet been added.

The main benefit, at least for me, is that this type makes it very clear that there's in fact two symbols, and choosing one is straightforward.

One downside of this approach is that is becomes harder for subclasses to call into the base implementation using a different symbol, for instance in Esm5ReflectionHost.getStaticProperty. This could be worked around by extracting the work of the base class into functions taking just a ts.Symbol.

3. Try to remove NgccReflectionHost.getClassSymbol altogether.

It should be noted that ngtsc's ReflectionHost interface never exposes ts.Symbol in the first place, it's only done in NgccReflectionHost. We could look into removing the exposure of class symbols altogether,. However, I would argue that this change alone would not really help preventing bugs like FW-1507, as internally we'd still have to deal with the existence of multiple ts.Symbols without making this an explicit concept in the codebase (as NgccClassSymbol would).