Patch backport of #38959 & #39267#39272
Closed
petebacondarwin wants to merge 19 commits intoangular:10.1.xfrom
Closed
Patch backport of #38959 & #39267#39272petebacondarwin wants to merge 19 commits intoangular:10.1.xfrom
petebacondarwin wants to merge 19 commits intoangular:10.1.xfrom
Conversation
…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
…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.
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
josephperrott
approved these changes
Oct 14, 2020
Member
josephperrott
left a comment
There was a problem hiding this comment.
LGTM
Review-For: global-approvers
Reviewed as global as this is a backport of an alreaady reviewed/approve change
josephperrott
approved these changes
Oct 14, 2020
Member
josephperrott
left a comment
There was a problem hiding this comment.
Reviewed-for: global-approvers
atscott
pushed a commit
that referenced
this pull request
Oct 14, 2020
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
atscott
pushed a commit
that referenced
this pull request
Oct 14, 2020
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
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) 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, 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
…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
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
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Patch backport of #38959 & #39267