Skip to content

Patch backport of #38959 & #39267#39272

Closed
petebacondarwin wants to merge 19 commits intoangular:10.1.xfrom
petebacondarwin:ngcc-inline-exports-patch
Closed

Patch backport of #38959 & #39267#39272
petebacondarwin wants to merge 19 commits intoangular:10.1.xfrom
petebacondarwin:ngcc-inline-exports-patch

Conversation

@petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Oct 14, 2020

Patch backport of #38959 & #39267

…lar#38959)

The message now also reports the name of the predicate function
that failed.

PR Close angular#38959
…onReference()` (angular#38959)

There is no need to check that the `ref.node` is of any particular type
because immediately after this check the entry is tested to see if it passes
`isClassDeclarationReference()`.

The only difference is that the error that is reported is slightly different
in the case that it is a `ref` but not one of the TS node types.

Previously:

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                className} is not a reference`
```

now

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                  className} is not a class`
```

Arguably the previous message was wrong, since this entry IS a reference
but is not a class.

PR Close angular#38959
The `isAssignment` and `isAssignmentStatement` are not used in this file.

PR Close angular#38959
…ar#38959)

This clarifies that this is specifically about statements of the form
`exports.<name> = <declaration>`, rather than a general export
statement such as `export class <ClassName> { ... }`.

PR Close angular#38959
…ngular#38959)

If the `symbol` for the given `node` does not exist then there is no
point in creating the `map`.

PR Close angular#38959
The `SIMPLE_CLASS_FILE` contained a `ChildClass` that had an
internal aliases implementation and extended a `SuperClass` base
class. The call to `__extends` was using the wrong argument for
the child class.

PR Close angular#38959
…38959)

UMD files export values by assigning them to an `exports` variable.
When evaluating expressions ngcc was failing to cope with expressions
like `exports.MyComponent`.

This commit fixes the `UmdReflectionHost.getDeclarationOfIdentifier()`
method to map the `exports` variable to the current source file.

PR Close angular#38959
Sometimes UMD exports appear in the following form:

```
exports.MyClass = alias1 = alias2 = <<declaration>>
```

Previously the declaration of the export would have been captured
as `alias1 = alias2 = <<declaration>>`, which the `PartialInterpreter`
would have failed on, since it cannot handle assignments.

Now we skip over these aliases capturing only the `<<declaration>>`
expression.

Fixes angular#38947

PR Close angular#38959
…aration()` (angular#38959)

The new function does not try to restrict the kind of AST node that it
finds, leaving that to the caller. This will make it more resuable in the
UMD reflection host.

PR Close angular#38959
…ngular#38959)

The protected helper functions can then be overridden by subclasses of the
Esm2015ReflectionHost.

PR Close angular#38959
…t to code format (angular#38959)

Previously `getDeclaration()` would only return the first node that matched
the name passed in and then assert the predicate on this single node.
It also only considered a subset of possible declaration types that we might
care about.

Now the function will parse the whole tree collecting an array of all the
nodes that match the name. It then filters this array based on the predicate
and only errors if the filtered array is empty.

This makes this function much more resilient to more esoteric code formats
such as UMD.

PR Close angular#38959
…38959)

This makes these tests more resilient to changes in the test code
structure. For example switching from

```
var SomeClass = <implementation>;
exports.SomeClass = SomeClass;
```

to

```
exports.SomeClass = <implementation>;
```

PR Close angular#38959
…r#38959)

Previously the `ConcreteDeclaration` and `InlineDeclaration` had
different properties for the underlying node type. And the `InlineDeclaration`
did not store a value that represented its declaration.

It turns out that a natural declaration node for an inline type is the
expression. For example in UMD/CommonJS this would be the `exports.<name>`
property access node.

So this expression is now used for the `node` of `InlineDeclaration` types
and the `expression` property is dropped.

To support this the codebase has been refactored to use a new `DeclarationNode`
type which is a union of `ts.Declaration|ts.Expression` instead of `ts.Declaration`
throughout.

PR Close angular#38959
…lar#38959)

Previously these tests were checking multiple specific expression
types. The new helper function is more general and will also support
`PropertyAccessExpression` nodes for `InlineDeclaration` types.

PR Close angular#38959
…8959)

Previously, any declarations that were defined "inline" were not
recognised by the `UmdReflectionHost`.

For example, the following syntax was completely unrecognized:

```ts
var Foo_1;
exports.Foo = Foo_1 = (function() {
  function Foo() {}
  return Foo;
})();
exports.Foo = Foo_1 = __decorate(SomeDecorator, Foo);
```

Such inline classes were ignored and not processed by ngcc.

This lack of processing led to failures in Ivy applications that relied
on UMD formats of libraries such as `syncfusion/ej2-angular-ui-components`.

Now all known inline UMD exports are recognized and processed accordingly.

Fixes angular#38947

PR Close angular#38959
…lar#38959)

Previously directive "queries" that relied upon a namespaced type

```ts
queries: {
  'mcontent': new core.ContentChild('test2'),
}
```

caused an error to be thrown. This is now supported.

PR Close angular#38959
…identity (angular#38959)

Previously the `node.name` property was only checked to ensure it was
defined. But that meant that it was a `ts.BindingName`, which also includes
`ts.BindingPattern`, which we do not support. But these helper methods were
forcefully casting the value to `ts.Identifier.

Now we also check that the `node.name` is actually an `ts.Identifier`.

PR Close angular#38959
@google-cla google-cla bot added the cla: yes label Oct 14, 2020
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler comp: ngcc target: patch This PR is targeted for the next patch release labels Oct 14, 2020
@ngbot ngbot bot modified the milestone: needsTriage Oct 14, 2020
@petebacondarwin petebacondarwin changed the title Ngcc inline exports patch Patch backport of #38959 Oct 14, 2020
…s differently

Some inline declarations are of the form:

```
exports.<name> = <implementation>;
```

In this case the declaration `node` is `exports.<name>`.
When interpreting such inline declarations we actually want
to visit the `implementation` expression rather than visiting
the declaration `node`.

This commit adds `implementation?: ts.Expression` to the
`InlineDeclaration` type and updates the interpreter to visit
these expressions as described above.
@petebacondarwin petebacondarwin changed the title Patch backport of #38959 Patch backport of #38959 & #39267 Oct 14, 2020
Previously, inline exports of the form `exports.foo = <implementation>;` were
being interpreted (by the ngtsc `PartialInterpeter`) as `Reference` objects.
This is not what is desired since it prevents the value of the export
from being unpacked, such as when analyzing `NgModule` declarations:

```
exports.directives = [Directive1, Directive2];

@NgImport({declarations: [exports.directives]})
class AppModule {}
```

In this example the interpreter would think that `exports.directives`
was a reference rather than an array that needs to be unpacked.

This bug was picked up by the ngcc-validation repository. See
angular/ngcc-validation#1990 and
https://circleci.com/gh/angular/ngcc-validation/17130
@pullapprove pullapprove bot requested a review from kyliau October 14, 2020 19:14
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 14, 2020
@ngbot
Copy link

ngbot bot commented Oct 14, 2020

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    failure status "ci/circleci: test_ivy_aot" is failing
    pending status "ci/circleci: test" is pending
    pending status "ci/circleci: test_aio" is pending
    pending status "ci/circleci: test_aio_local" is pending
    pending status "ci/circleci: test_aio_local_viewengine" is pending
    pending status "ci/circleci: test_docs_examples" is pending
    pending status "ci/circleci: test_docs_examples_viewengine" is pending
    pending status "google3" is pending
    pending status "pullapprove" is pending
    pending missing required status "ci/circleci: publish_snapshot"
    pending 2 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Review-For: global-approvers

Reviewed as global as this is a backport of an alreaady reviewed/approve change

Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: global-approvers

atscott pushed a commit that referenced this pull request Oct 14, 2020
…) (#39272)

The message now also reports the name of the predicate function
that failed.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…onReference()` (#38959) (#39272)

There is no need to check that the `ref.node` is of any particular type
because immediately after this check the entry is tested to see if it passes
`isClassDeclarationReference()`.

The only difference is that the error that is reported is slightly different
in the case that it is a `ref` but not one of the TS node types.

Previously:

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                className} is not a reference`
```

now

```
`Value at position ${idx} in the NgModule.${arrayName} of ${
                  className} is not a class`
```

Arguably the previous message was wrong, since this entry IS a reference
but is not a class.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
The `isAssignment` and `isAssignmentStatement` are not used in this file.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
… (#39272)

This clarifies that this is specifically about statements of the form
`exports.<name> = <declaration>`, rather than a general export
statement such as `export class <ClassName> { ... }`.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…38959) (#39272)

If the `symbol` for the given `node` does not exist then there is no
point in creating the `map`.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
The `SIMPLE_CLASS_FILE` contained a `ChildClass` that had an
internal aliases implementation and extended a `SuperClass` base
class. The call to `__extends` was using the wrong argument for
the child class.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…39272)

UMD files export values by assigning them to an `exports` variable.
When evaluating expressions ngcc was failing to cope with expressions
like `exports.MyComponent`.

This commit fixes the `UmdReflectionHost.getDeclarationOfIdentifier()`
method to map the `exports` variable to the current source file.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
Sometimes UMD exports appear in the following form:

```
exports.MyClass = alias1 = alias2 = <<declaration>>
```

Previously the declaration of the export would have been captured
as `alias1 = alias2 = <<declaration>>`, which the `PartialInterpreter`
would have failed on, since it cannot handle assignments.

Now we skip over these aliases capturing only the `<<declaration>>`
expression.

Fixes #38947

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…aration()` (#38959) (#39272)

The new function does not try to restrict the kind of AST node that it
finds, leaving that to the caller. This will make it more resuable in the
UMD reflection host.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…38959) (#39272)

The protected helper functions can then be overridden by subclasses of the
Esm2015ReflectionHost.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…t to code format (#38959) (#39272)

Previously `getDeclaration()` would only return the first node that matched
the name passed in and then assert the predicate on this single node.
It also only considered a subset of possible declaration types that we might
care about.

Now the function will parse the whole tree collecting an array of all the
nodes that match the name. It then filters this array based on the predicate
and only errors if the filtered array is empty.

This makes this function much more resilient to more esoteric code formats
such as UMD.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…39272)

This makes these tests more resilient to changes in the test code
structure. For example switching from

```
var SomeClass = <implementation>;
exports.SomeClass = SomeClass;
```

to

```
exports.SomeClass = <implementation>;
```

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
#39272)

Previously the `ConcreteDeclaration` and `InlineDeclaration` had
different properties for the underlying node type. And the `InlineDeclaration`
did not store a value that represented its declaration.

It turns out that a natural declaration node for an inline type is the
expression. For example in UMD/CommonJS this would be the `exports.<name>`
property access node.

So this expression is now used for the `node` of `InlineDeclaration` types
and the `expression` property is dropped.

To support this the codebase has been refactored to use a new `DeclarationNode`
type which is a union of `ts.Declaration|ts.Expression` instead of `ts.Declaration`
throughout.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…) (#39272)

Previously these tests were checking multiple specific expression
types. The new helper function is more general and will also support
`PropertyAccessExpression` nodes for `InlineDeclaration` types.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…39272)

Previously, any declarations that were defined "inline" were not
recognised by the `UmdReflectionHost`.

For example, the following syntax was completely unrecognized:

```ts
var Foo_1;
exports.Foo = Foo_1 = (function() {
  function Foo() {}
  return Foo;
})();
exports.Foo = Foo_1 = __decorate(SomeDecorator, Foo);
```

Such inline classes were ignored and not processed by ngcc.

This lack of processing led to failures in Ivy applications that relied
on UMD formats of libraries such as `syncfusion/ej2-angular-ui-components`.

Now all known inline UMD exports are recognized and processed accordingly.

Fixes #38947

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…) (#39272)

Previously directive "queries" that relied upon a namespaced type

```ts
queries: {
  'mcontent': new core.ContentChild('test2'),
}
```

caused an error to be thrown. This is now supported.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…identity (#38959) (#39272)

Previously the `node.name` property was only checked to ensure it was
defined. But that meant that it was a `ts.BindingName`, which also includes
`ts.BindingPattern`, which we do not support. But these helper methods were
forcefully casting the value to `ts.Identifier.

Now we also check that the `node.name` is actually an `ts.Identifier`.

PR Close #38959

PR Close #39272
atscott pushed a commit that referenced this pull request Oct 14, 2020
…s differently (#39272)

Some inline declarations are of the form:

```
exports.<name> = <implementation>;
```

In this case the declaration `node` is `exports.<name>`.
When interpreting such inline declarations we actually want
to visit the `implementation` expression rather than visiting
the declaration `node`.

This commit adds `implementation?: ts.Expression` to the
`InlineDeclaration` type and updates the interpreter to visit
these expressions as described above.

PR Close #39272
@josephperrott josephperrott removed the request for review from kyliau October 14, 2020 19:59
atscott pushed a commit that referenced this pull request Oct 14, 2020
…39272)

Previously, inline exports of the form `exports.foo = <implementation>;` were
being interpreted (by the ngtsc `PartialInterpeter`) as `Reference` objects.
This is not what is desired since it prevents the value of the export
from being unpacked, such as when analyzing `NgModule` declarations:

```
exports.directives = [Directive1, Directive2];

@NgImport({declarations: [exports.directives]})
class AppModule {}
```

In this example the interpreter would think that `exports.directives`
was a reference rather than an array that needs to be unpacked.

This bug was picked up by the ngcc-validation repository. See
angular/ngcc-validation#1990 and
https://circleci.com/gh/angular/ngcc-validation/17130

PR Close #39272
@atscott atscott closed this Oct 14, 2020
@petebacondarwin petebacondarwin deleted the ngcc-inline-exports-patch branch October 14, 2020 20:13
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants