Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All ngcc migrations #33362

Closed
wants to merge 7 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Oct 23, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 23, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Oct 23, 2019
Copy link
Member

JoostK left a comment

First round of comments (for today). Overall this is a bit light on tests.

// This migration looks at NgModules and considers the directives (and pipes) it declares.
// It verifies that these classes have decorators.
Comment on lines +23 to +22

This comment has been minimized.

Copy link
@JoostK

JoostK Oct 23, 2019

Member

I thought we discussed cases where undecorated classes might exist in libraries without registering them in an NgModule. Although this sounds like a pathological use case to me, I thought @petebacondarwin was able to convince you 😉

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 23, 2019

Author Contributor

I thought through this and came to the realization that it's not actually possible. VE would consider those classes as decorated, and would generate an error ("could not determine NgModule for ...") if they were not part of a module.

So I don't think that case actually occurs in reality.

}
}

function compileNodeModuleToFs(fs: FileSystem, pkgName: string, pkg: Package): void {

This comment has been minimized.

Copy link
@JoostK

JoostK Oct 23, 2019

Member

I love this, this is super helpful for having easy to write and easy to maintain tests 😍

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 23, 2019

Author Contributor

I want to expand it over time, so you can start with TS code and ask for a node_module layout with some or all of the possible formats, rolled up to UMD, maybe even minified, etc.

fs.resolve(`/node_modules/${pkgName}${inFileBase}.d.ts`),
compileFs.readFile(compileFs.resolve(`${inFileBase}.d.ts`)));
const jsContents = compileFs.readFile(compileFs.resolve(`${inFileBase}.js`));
fs.writeFile(fs.resolve(`/node_modules/${pkgName}${inFileBase}.js`), jsContents);

This comment has been minimized.

Copy link
@JoostK

JoostK Oct 23, 2019

Member

Could you ensure there's a slash separating inFileBase from pkgName?

this.fs.writeFile(this.fs.resolve(fileName), data);
}

getCurrentDirectory(): string { return '/'; }

This comment has been minimized.

Copy link
@JoostK

JoostK Oct 23, 2019

Member

Consider using this.fs.pwd();.

packages/compiler-cli/src/ngtsc/transform/src/api.ts Outdated Show resolved Hide resolved
];

/**
* Merges the definition from a super class to a sub class.

This comment has been minimized.

Copy link
@JoostK

JoostK Oct 23, 2019

Member

Mentioning merge semantics is confusing here, given the feature's name.

@@ -104,6 +104,9 @@ function addFeatures(
if (meta.usesInheritance) {
features.push(o.importExpr(R3.InheritDefinitionFeature));
}
if (meta.fullInheritance) {

This comment has been minimized.

Copy link
@JoostK

JoostK Oct 23, 2019

Member

If CopyDefinitionFeature always requires InheritDefinitionFeature (which I believe is the case judging on the feature's impl), I would prefer that to be explicit:

if (meta.fullInheritance) {
  features.push(o.importExpr(R3.InheritDefinitionFeature));
  features.push(o.importExpr(R3.CopyDefinitionFeature));
} else if (meta.usesInheritance) {
  features.push(o.importExpr(R3.InheritDefinitionFeature));
}

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 23, 2019

Author Contributor

I think in general we need to make inheritance an enum:

enum R3DecoratorInheritance {
  None,
  Limited, /* decorated fields are inherited... inputs, outputs, etc) */
  Full, /* all aspects of the base class are inherited */
}

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 24, 2019

Author Contributor

@kara please review the last two commits, especially the runtime side.

@alxhub alxhub force-pushed the alxhub:ngcc/migrations branch from 0750fc8 to 4f22811 Oct 23, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

Copy link
Member

gkalpak left a comment

I'd also like to see some more tests. Otherwise, everything looks reasonable (afaict 😁) 👍

@@ -23,6 +23,14 @@ export function hasDirectiveDecorator(host: MigrationHost, clazz: ClassDeclarati
return host.metadata.getDirectiveMetadata(ref) !== null || host.metadata.isAbstractDirective(ref);
}

/**
* Returns true if the `clazz` is decorated as a `Directive` or `Component`.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

as a `Directive` or `Component` --> as a `Pipe`

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 24, 2019

Author Contributor

👍

NONE = 0x0,

/**
* Indicates that this decorator should fully inherited from its parent at runtime. In addition

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

inherited --> inherit(?)

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 24, 2019

Author Contributor

👍

* Its primary effect is to cause the `CopyDefinitionFeature` to be applied to the definition
* being compiled. See that class for more information.
*/
FULL_INHERITANCE = 0x00000001,

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

Why not 0x1 (or 0x00000000 above 😁)?

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 24, 2019

Author Contributor

Gotta make room for the other flags!

Honestly, it's so things line up nicely when we reach 0x10. Though that'll never happen.


maybeMigrate(ref: Reference<ClassDeclaration>, host: MigrationHost): ts.Diagnostic|null {
if (hasDirectiveDecorator(host, ref.node) || hasPipeDecorator(host, ref.node)) {
// Stop if one of the classes in the chain is actually decorated with @Directive.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

May I add or @Component or @Pipe? 😛

}


// Success!

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

Yay 🎉 👏 🍾


// Examine each of the declarations to see if it needs to be migrated.
for (const decl of moduleMeta.declarations) {
const diag = this.maybeMigrate(decl, host);

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

It seems that this can never return anything but null.

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 24, 2019

Author Contributor

Failure is not an option.

[relPath: string]: string;
};

export function genNodeModules(def: NodeModulesDef): void {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 24, 2019

Member

Please add a comment here and/or on compileNodeModuleToFs() to explain what the function does.

This comment has been minimized.

Copy link
@alxhub

alxhub Oct 24, 2019

Author Contributor

Done :)

Copy link
Contributor

kara left a comment

A few small things

@kara kara marked this pull request as ready for review Oct 24, 2019
@kara kara requested review from angular/fw-compiler as code owners Oct 24, 2019
@alxhub alxhub force-pushed the alxhub:ngcc/migrations branch from 4f22811 to 5144be2 Oct 24, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 24, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 24, 2019
@kara
kara approved these changes Oct 24, 2019
Copy link
Contributor

kara left a comment

LGTM

@alxhub alxhub force-pushed the alxhub:ngcc/migrations branch 2 times, most recently from ae402b5 to e015bdd Oct 24, 2019
JoostK added 2 commits Oct 22, 2019
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.
@alxhub alxhub force-pushed the alxhub:ngcc/migrations branch from e015bdd to d3998f0 Oct 24, 2019
@alxhub alxhub added cla: yes and removed cla: no labels Oct 24, 2019
@kara

This comment has been minimized.

Copy link
Contributor

kara commented Oct 25, 2019

merge-assistance: global approval

JoostK and others added 5 commits Oct 20, 2019
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.
This commit adds CopyDefinitionFeature, which supports the case where an
entire decorator (@component or @directive) is inherited from parent to
child.

The existing inheritance feature, InheritDefinitionFeature, supports merging
of parent and child definitions when both were originally present. This
merges things like inputs, outputs, host bindings, etc.

CopyDefinitionFeature, on the other hand, compensates for a definition that
was missing entirely on the child class, by copying fields that aren't
ordinarily inherited (like the template function itself).

This feature is intended to only be used as part of ngcc code generation.
In Angular View Engine, there are two kinds of decorator inheritance:

1) both the parent and child classes have decorators

This case is supported by InheritDefinitionFeature, which merges some fields
of the definitions (such as the inputs or queries).

2) only the parent class has a decorator

If the child class is missing a decorator, the compiler effectively behaves
as if the parent class' decorator is applied to the child class as well.
This is the "undecorated child" scenario, and this commit adds a migration
to ngcc to support this pattern in Ivy.

This migration has 2 phases. First, the NgModules of the application are
scanned for classes in 'declarations' which are missing decorators, but
whose base classes do have decorators. These classes are the undecorated
children. This scan is performed recursively, so even if a declared class
has a base class that itself inherits a decorator, this case is handled.

Next, a synthetic decorator (either @component or @directive) is created
on the child class. This decorator copies some critical information such
as 'selector' and 'exportAs', as well as supports any decorated fields
(@input, etc). A flag is passed to the decorator compiler which causes a
special feature `CopyDefinitionFeature` to be included on the compiled
definition. This feature copies at runtime the remaining aspects of the
parent definition which `InheritDefinitionFeature` does not handle,
completing the "full" inheritance of the child class' decorator from its
parent class.
@alxhub alxhub force-pushed the alxhub:ngcc/migrations branch from d3998f0 to f1b6302 Oct 25, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 25, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 25, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 25, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 25, 2019

FYI, there are build failures in g3 with the changes from this PR, adding "blocked" label.

@AndrewKushnir AndrewKushnir added cla: yes and removed cla: no labels Oct 25, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Oct 25, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 25, 2019

New set of presubmits:

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Oct 25, 2019

FYI, VE and Ivy presubmits are successful.

AndrewKushnir added a commit that referenced this pull request Oct 25, 2019
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.

PR Close #33362
AndrewKushnir added a commit that referenced this pull request Oct 25, 2019
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.

PR Close #33362
AndrewKushnir added a commit that referenced this pull request Oct 25, 2019
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.

PR Close #33362
AndrewKushnir added a commit that referenced this pull request Oct 25, 2019
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.

PR Close #33362
AndrewKushnir added a commit that referenced this pull request Oct 25, 2019
This commit adds CopyDefinitionFeature, which supports the case where an
entire decorator (@component or @directive) is inherited from parent to
child.

The existing inheritance feature, InheritDefinitionFeature, supports merging
of parent and child definitions when both were originally present. This
merges things like inputs, outputs, host bindings, etc.

CopyDefinitionFeature, on the other hand, compensates for a definition that
was missing entirely on the child class, by copying fields that aren't
ordinarily inherited (like the template function itself).

This feature is intended to only be used as part of ngcc code generation.

PR Close #33362
AndrewKushnir added a commit that referenced this pull request Oct 25, 2019
In Angular View Engine, there are two kinds of decorator inheritance:

1) both the parent and child classes have decorators

This case is supported by InheritDefinitionFeature, which merges some fields
of the definitions (such as the inputs or queries).

2) only the parent class has a decorator

If the child class is missing a decorator, the compiler effectively behaves
as if the parent class' decorator is applied to the child class as well.
This is the "undecorated child" scenario, and this commit adds a migration
to ngcc to support this pattern in Ivy.

This migration has 2 phases. First, the NgModules of the application are
scanned for classes in 'declarations' which are missing decorators, but
whose base classes do have decorators. These classes are the undecorated
children. This scan is performed recursively, so even if a declared class
has a base class that itself inherits a decorator, this case is handled.

Next, a synthetic decorator (either @component or @directive) is created
on the child class. This decorator copies some critical information such
as 'selector' and 'exportAs', as well as supports any decorated fields
(@input, etc). A flag is passed to the decorator compiler which causes a
special feature `CopyDefinitionFeature` to be included on the compiled
definition. This feature copies at runtime the remaining aspects of the
parent definition which `InheritDefinitionFeature` does not handle,
completing the "full" inheritance of the child class' decorator from its
parent class.

PR Close #33362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.