Skip to content

feat(ngcc): execute schematic-like migrations in ngcc - #33279

Closed
JoostK wants to merge 8 commits into
angular:masterfrom
JoostK:ngcc-missing-injectable-migration
Closed

JoostK wants to merge 8 commits into
angular:masterfrom
JoostK:ngcc-missing-injectable-migration

Conversation

@JoostK

@JoostK JoostK commented Oct 20, 2019

Copy link
Copy Markdown
Member

This PR contains several commits to introduce schematic-like migration in ngcc. More details can be found in each commit description.

@JoostK JoostK added feature Label used to distinguish feature request from other issues target: major This PR is targeted for the next major release comp: ivy labels Oct 20, 2019
@ngbot ngbot Bot modified the milestone: needsTriage Oct 20, 2019
@JoostK

This comment has been minimized.

@JoostK
JoostK force-pushed the ngcc-missing-injectable-migration branch 2 times, most recently from b4659f9 to e335292 Compare October 21, 2019 23:28
gkalpak added a commit to gkalpak/ngcc-validation that referenced this pull request Oct 22, 2019
@JoostK
JoostK force-pushed the ngcc-missing-injectable-migration branch from e335292 to 0be9bde Compare October 22, 2019 18:47
@JoostK
JoostK marked this pull request as ready for review October 22, 2019 18:53
@JoostK
JoostK requested review from a team October 22, 2019 18:53
@JoostK

JoostK commented Oct 22, 2019 •

Copy link
Copy Markdown
Member Author

After having tested within the ngcc-validation project in angular/ngcc-validation#439 (thanks @gkalpak!) two issues were identified:

  1. synthetic decorators did get emitted as metadata, which should not be the case. This has been addressed by preventing the internal decorators cache of Esm2015ReflectionHost from being clobbered with outside changes, such as injecting synthetic decorators. With this change, synthetic decorators no longer leak into the real decorators so won't be emitted as metadata.
  2. Abstract directives could be migrated despite already having a @Directive decorator, causing an error. This error was addressed by updating hasDirectiveDEcorator to also consider MetadataReader.isAbstractDirective, and tests have been adjusted to assert that no errors occurred. Note that this change is obsolete once fix(ivy): support abstract directives in template type checking #33131 has landed.

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 gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 🤷‍♂

@gkalpak gkalpak Oct 23, 2019 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to be determine --> to be determined/to determine

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the point of having node point to the node for which the decorator was synthesized? Is it used anywhere?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add some assertions about what happens to IntermediateClass.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.
@JoostK
JoostK force-pushed the ngcc-missing-injectable-migration branch from 0be9bde to ebf596c Compare October 23, 2019 19:51
@JoostK

JoostK commented Oct 24, 2019

Copy link
Copy Markdown
Member Author

Closing as these commits are part of #33362 to be landed at once.

@JoostK JoostK closed this Oct 24, 2019
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes feature Label used to distinguish feature request from other issues target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants