Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
b4659f9 to
e335292
Compare
e335292 to
0be9bde
Compare
|
After having tested within the ngcc-validation project in angular/ngcc-validation#439 (thanks @gkalpak!) two issues were identified:
I am fairly confident that the migrations now operate as they should. The two packages that regressed in the test run are now building successfully for me locally. |
gkalpak
left a comment
There was a problem hiding this comment.
Great stuff 💯
I wish there was some way to share logic with the schematics migrations, but I guess the "infrastructure" is too different for that 🤷♂
There was a problem hiding this comment.
to be determine --> to be determined/to determine
There was a problem hiding this comment.
What is the point of having node point to the node for which the decorator was synthesized? Is it used anywhere?
There was a problem hiding this comment.
Decorator.node is primarily used for producing diagnostics (for its location) so it would be quite cumbersome if this could be null. For synthesized decorators we don't expect diagnostics to be created, so from that perspective this shouldn't matter much, however I liked using the migrated class' identifier as a fallback.
There was a problem hiding this comment.
Could you add some assertions about what happens to IntermediateClass.
There was a problem hiding this comment.
This paragraph confuses me a bit. By reading it, I thought that we need the @Injectable decorator no matter what. By looking at the code, it seems that @Injectable is needed only if the class does not have @Directive, @Component or @Pipe.
ngcc has an internal cache of computed decorator information for reflected classes, which could previously be mutated by consumers of the reflection host. With the ability to inject synthesized decorators, such decorators would inadvertently be added into the array of decorators that was owned by the internal cache of the reflection host, incorrectly resulting in synthesized decorators to be considered real decorators on a class. This commit fixes the issue by cloning the cached array before returning it.
A class that is provided as Angular service is required to have an `@Injectable()` decorator so that the compiler generates its injectable definition for the runtime. Applications are automatically migrated using the "missing-injectable" schematic, however libraries built for older version of Angular may not yet satisfy this requirement. This commit ports the "missing-injectable" schematic to a migration that is ran when ngcc is processing a library. This ensures that any service that is provided from an NgModule or Directive/Component will have an `@Injectable()` decorator.
In ngcc's migration system, synthetic decorators can be injected into a compilation to ensure that certain classes are compiled with Angular logic, where the original library code did not include the necessary decorators. Prior to this change, synthesized decorators would have a fake AST structure as associated node and a made-up identifier. In theory, this may introduce issues downstream: 1) a decorator's node is used for diagnostics, so it must have position information. Having fake AST nodes without a position is therefore a problem. Note that this is currently not a problem in practice, as injected synthesized decorators would not produce any diagnostics. 2) the decorator's identifier should refer to an imported symbol. Therefore, it is required that the symbol is actually imported. Moreover, bundle formats such as UMD and CommonJS use namespaces for imports, so a bare `ts.Identifier` would not be suitable to use as identifier. This was also not a problem in practice, as the identifier is only used in the `setClassMetadata` generated code, which is omitted for synthetically injected decorators. To remedy these potential issues, this commit makes a decorator's identifier optional and switches its node over from a fake AST structure to the class' name.
Previously, the (currently disabled) undecorated parent migration in ngcc would produce errors when a base class could not be determined statically or when a class extends from a class in another package. This is not ideal, as it would cause the library to fail compilation without a workaround, whereas those problems are not guaranteed to cause issues. Additionally, inheritance chains were not handled. This commit reworks the migration to address these limitations.
When upgrading an Angular application to a new version using the Angular CLI, built-in schematics are being run to update user code from deprecated patterns to the new way of working. For libraries that have been built for older versions of Angular however, such schematics have not been executed which means that deprecated code patterns may still be present, potentially resulting in incorrect behavior. Some of the logic of schematics has been ported over to ngcc migrations, which are automatically run on libraries. These migrations achieve the same goal of the regular schematics, but operating on published library sources instead of used code.
0be9bde to
ebf596c
Compare
|
Closing as these commits are part of #33362 to be landed at once. |
|
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 PR contains several commits to introduce schematic-like migration in ngcc. More details can be found in each commit description.