# 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.Symbol`s, 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`][1]. 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.Symbol`s without making this an explicit concept in the codebase (as `NgccClassSymbol` would).

[1]: https://github.com/angular/angular/blob/04d4fea3e8784ea81916b24f3b153331967bad71/packages/compiler-cli/ngcc/src/host/esm5_host.ts#L499-L519