fix(compiler-cli): don't unnecessarily throw on StaticSymbols - #12158

Could you add a test for this new behavior? In https://github.com/angular/angular/blob/master/modules/@angular/compiler-cli/test/static_reflector_spec.ts...

This change is wrong. The reason that this code checks using instanceof is because the static reflector assumes that all the static symbols it receives in its API were produced by the static reflector. This is because it uses object identity to cache information and determine type identity. The bug is not that we are checking for instanceof instead of using isStaticSymbol, rather, it is that someone is passing static symbol that wasn't produced by the static reflector. In this case the test is doing what it was intended to do, report this case.

got it, thanks for the explanation @chuckjaz

I'll dig deeper and find the source of static symbols not created by the static reflector and update this PR accordingly

So from digging around deeper I see what is going on.

The objects were originally static symbols created by the StaticSymbol constructor, but then were cloned in the mapStringMap function (which is called by simplify in the parameters method of the static reflector) which is where the instanceof check breaks, as well as proper ability to cache by reference.

Looking deeper into why this happens to only some providers in some situations and to why the simplify functions that are being used exist in the first place and if the cloning can be circumvented or if it is a deeper issue that StaticSymbols are passed to the simplify function in the first place

Thanks for looking into this! StaticSymbol should be a special case in the evaluator. Try adding the lines:

        if (expression instanceof StaticSymbol) {
          return expression;
        }

just after the test for Array in simplify(). This will prevent the static reflector from trying to clone a StaticSymbol, treating it like a primitive instead of a hash map.

If you find how a StaticSymbol got in a value passed to simplify() I would be curious. That shouldn't happen but if it does we shouldn't damage it.

@chuckjaz agreed, and thanks!

My solution was identical to yours but I added the check below the check for primitives, if it's important that it be below the check for Array just let me know and I'll happily move it

So far I am stumped as well how only in rare cases static symbols are passed to simplify (it's within some deep recursion in some edge case situations it seems) but I'll definitely let you know if I figure it out!

Also, just added a test for this new check as well 👍

We are cross talking! No need for it being after Array really. It is slightly faster there (in that StaticSymbols are less common than Arrays) but not really noticeably so. Only move it if you feel so inclined.

@chuckjaz makes perfect sense, I went a head and pushed one small update with that extra optimization 👍

Could you then also export the StaticSymbol here as well and drop the declaration in static_reflector? You will probably need to add members to the one in @angular/compiler...

Found the issue.

When recursively processing/simplifying annotation metadata StaticSymbols need to be kept in tact rather than treated as plain string maps and cloned

This was happening to us in universal for any private import we made from platform-browser using the format we see from the this repo must be used to properly import private exports (i.e. using the explicit typeof x type annotations) which we do in universal here.

I'm guessing this is just a byproduct of the parsing being a bit different when using that special import/export format required for private exports.

I've updated this PR accordingly to check for StaticSymbol before further processing in the simplifyInContext method which I've verified with this fix universal now builds and runs as expected with AOT

Happy to make any further updates or refactors desired as well, just let me know, and thanks for taking a look at this @tbosch and @chuckjaz!

Can you add a test to static_reflector_spec.ts that verifies that simplify() will not clone a StaticSymbol? Something like:

  it('should not clone a static symbol', () => {
    const symbol = new StaticSymbol('/tmp/src/some_file.ts', 'SomeSymbol');
    expect(simplify(symbol, symbol)).toBe(symbol);
  });

@chuckjaz just did in a separate commit 👍

if you want me to squash it with the first, just let me know

Up to you. Tobias will probably squash and merge anyway.

No need for this to be a separate commit. Just squash them.

@chuckjaz squashed 👍