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

FW-517 & FW-536: private directive issues #27743

Closed
wants to merge 6 commits into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Dec 19, 2018

No description provided.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 19, 2018

@@ -28,7 +29,7 @@ export class FlatIndexGenerator implements ShimGenerator {
// If there's only one .ts file in the program, it's the entry. Otherwise, look for the shortest
// (in terms of characters in the filename) file that ends in /index.ts. The second behavior is
// deprecated; users should always explicitly specify a single .ts entrypoint.
const tsFiles = files.filter(isNonDeclarationTsFile);
const tsFiles = files.filter(file => !file.endsWith('.d.ts'));

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Dec 19, 2018

Member

Why de-abstract this?

This comment has been minimized.

Copy link
@alxhub

alxhub Dec 19, 2018

Author Contributor

Fixed :)

@alxhub alxhub force-pushed the alxhub:ngtsc-private-names branch from fa37776 to a2e7dd7 Dec 19, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 20, 2018

You can preview a2e7dd7 at https://pr27743-a2e7dd7.ngbuilds.io/.

* found in the LICENSE file at https://angular.io/license
*/

import * as ts from 'typescript';

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Unused import?

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

Fixed!

@@ -28,7 +30,7 @@ export class FlatIndexGenerator implements ShimGenerator {
// If there's only one .ts file in the program, it's the entry. Otherwise, look for the shortest
// (in terms of characters in the filename) file that ends in /index.ts. The second behavior is
// deprecated; users should always explicitly specify a single .ts entrypoint.
const tsFiles = files.filter(isNonDeclarationTsFile);
const tsFiles = files.filter(file => !isDtsPath(file));

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Is it guaranteed that files does not contains any .js files?

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

No, it's not. Fixed.

export {FormControlDirective} from './directives/reactive_directives/form_control_directive';
export {FormControlName} from './directives/reactive_directives/form_control_name';
export {FormGroupDirective} from './directives/reactive_directives/form_group_directive';
export {FormArrayName} from './directives/reactive_directives/form_group_name';
export {FormGroupName} from './directives/reactive_directives/form_group_name';
export {NgSelectOption, SelectControlValueAccessor} from './directives/select_control_value_accessor';
export {SelectMultipleControlValueAccessor} from './directives/select_multiple_control_value_accessor';
export {NgSelectMultipleOption as ɵNgSelectMultipleOption} from './directives/select_multiple_control_value_accessor';

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Join with the above export?

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

I kept them separate specifically because one is a public export and the other is private.

registerOnTouched(fn: () => void): void;
setDisabledState(isDisabled: boolean): void;
writeValue(value: any): void;
}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Now that these classes are public API, it would be better to have more descriptive param names.

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

Even the public ControlValueAccessors use these same parameter names, so I think this is a change that would have to happen forms-wide, and out of scope for this refactor.

@@ -19,6 +19,8 @@ import * as ts from 'typescript';
*/
export interface BundleProgram {
program: ts.Program;
host: ts.CompilerHost;
options: ts.CompilerOptions;

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Order doesn't match all other places 😁 😇

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

Switched!

// Neither is the module which declares it - meaning the directive is not visible here.
@NgModule({declarations: [Dir], exports: [Dir]})
class DirModule {}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

What would happen if Dir was exported but DirModule wasn't?
What would happen if DirModule was imported by Module but not exported?
Might be worth adding tests for these usecases as well.

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 8, 2019

Author Contributor

Added!

export class ReferenceGraph<T = ts.Declaration> {
private references = new Map<T, Set<T>>();

constructor() {}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Redundant empty constructor.

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

Removed.

return null;
} else {
// Look through the outgoing edges of `source`.
// TODO(alxhub): use proper iteration when build.sh is removed. (#27762)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Could you at least use for ... of Array.from(set) (until build.sh is removed)?

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

This would allocate, which I'm trying to avoid.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jan 8, 2019

Member

build.sh has been removed in #27937 fwiw 😉

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 8, 2019

Author Contributor

🎉🍾🎆

I'll go through and convert back to beautiful native iteration.

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 8, 2019

Author Contributor

Noooooo!!

The legacy build on CircleCI is basically build.sh reincarnated. It has the same issue 😭

Reference<ts.Declaration>;
}

export class NpmReferenceResolver implements ReferenceResolver {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

Possibly a n00b question, but...why Npm?

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

A better name would be the TsReferenceResolver - renamed.

@NgModule({
declarations: [Cmp],
// Multiple imports of the same module used to result in duplicate directive references
// in the output.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 21, 2018

Member

This comment is irrelevant here.

This comment has been minimized.

Copy link
@alxhub

alxhub Jan 7, 2019

Author Contributor

Fixed!

@gkalpak gkalpak referenced this pull request Dec 21, 2018

Closed

feat(ivy): implement listing lazy routes in `ngtsc` #27697

5 of 7 tasks complete
@AndrewKushnir
Copy link
Contributor

AndrewKushnir left a comment

Looks great, thanks @alxhub.

@alxhub alxhub force-pushed the alxhub:ngtsc-private-names branch from a2e7dd7 to 44d4413 Jan 7, 2019

@alxhub alxhub requested review from angular/fw-compiler as code owners Jan 7, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Jan 8, 2019

@AndrewKushnir you're correct in that it makes this particular "Analyzer" function a bit differently than the others. I actually think this version is correct. The other analyzers have a signature like:

class Analyzer {
  constructor(private checker: ts.TypeChecker) {}
  analyzeProgram(program: ts.Program) {}
}

This is arguably incorrect. A ts.TypeChecker is created from a ts.Program - it makes no sense to analyze one program with the TypeChecker from another. So even though this API looks like a single instance of the analyzer can run over multiple programs, this would not work in practice, and analyzeProgram must be called with the same Program as was used to get the TypeChecker the analyzer was constructed with.

Thus, I think my refactor actually makes the API better, and the other analyzer should follow suit.

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 8, 2019

alxhub added some commits Dec 7, 2018

refactor(ivy): move the flat module index generator to its own package
This commit moves the FlatIndexGenerator to its own package, in preparation
to expand its capabilities and support re-exporting of private declarations
from NgModules.
refactor(ivy): prep ngtsc and ngcc for upcoming import resolution work
Upcoming work to implement import resolution will change the dependencies
of some higher-level classes in ngtsc & ngcc. This necessitates changes in
how these classes are created and the lifecycle of the ts.Program in ngtsc
& ngcc.

To avoid complicating the implementation work with refactoring as a result
of the new dependencies, the refactoring is performed in this commit as a
separate prepatory step.

In ngtsc, the testing harness is modified to allow easier access to some
aspects of the ts.Program.

In ngcc, the main change is that the DecorationAnalyzer is created with the
ts.Program as a constructor parameter. This is not a lifecycle change, as
it was previously created with the ts.TypeChecker which is derived from the
ts.Program anyways. This change requires some reorganization in ngcc to
accommodate, especially in testing harnesses where DecorationAnalyzer is
created manually in a number of specs.
refactor(ivy): split apart the 'metadata' package in the ngtsc compiler
This refactoring moves code around between a few of the ngtsc subpackages,
with the goal of having a more logical package structure. Additional
interfaces are also introduced where they make sense.

The 'metadata' package formerly contained both the partial evaluator,
the TypeScriptReflectionHost as well as some other reflection functions,
and the Reference interface and various implementations. This package
was split into 3 parts.

The partial evaluator now has its own package 'partial_evaluator', and
exists behind an interface PartialEvaluator instead of a top-level
function. In the future this will be useful for reducing churn as the
partial evaluator becomes more complicated.

The TypeScriptReflectionHost and other miscellaneous functions have moved
into a new 'reflection' package. The former 'host' package which contained
the ReflectionHost interface and associated types was also merged into this
new 'reflection' package.

Finally, the Reference APIs were moved to the 'imports' package, which will
consolidate all import-related logic in ngtsc.

@alxhub alxhub force-pushed the alxhub:ngtsc-private-names branch from 44d4413 to da6414b Jan 8, 2019

@alxhub alxhub requested a review from kara Jan 8, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 8, 2019

@alxhub alxhub force-pushed the alxhub:ngtsc-private-names branch 2 times, most recently from 1ed0cf4 to a457e09 Jan 8, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 8, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 8, 2019

@kara

kara approved these changes Jan 8, 2019

Copy link
Contributor

kara left a comment

LGTM

@gkalpak

gkalpak approved these changes Jan 8, 2019

Copy link
Member

gkalpak left a comment

LGTM (with a couple of minor comments)

Show resolved Hide resolved packages/compiler-cli/src/ngtsc/imports/src/references.ts
* This is used for returning references to things like method declarations, which are not directly
* referenceable.
*/
export class NodeReference<T extends ts.Node = ts.Node> extends Reference<T> {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jan 8, 2019

Member

Then why doesn't it have expressable = false? 😕

*
* This is a reified type to allow the circular reference of `ResolvedValue` -> `ResolvedValueArray`
* ->
* `ResolvedValue`.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jan 8, 2019

Member

This is marked as resolved, but I still see this unexpectedly formatted 😕
I would expect `ResolvedValue` to be on the same line as the preceeding ->.

(Feel free to ignore my comment, if you have intentionally formatted it like this.)

@alxhub alxhub referenced this pull request Jan 8, 2019

Closed

Fix g3 core imports #27998

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Jan 8, 2019

@gkalpak

I'm mentioning it because there is an i here, but there is no i is several other places and I don't expect it to make any difference (and it is indeed existing code being moved around), so 👍

You're correct, and this should be unified, but it's outside the scope of this PR for now.

Then why doesn't it have expressable = false? 😕

It should - I'll fix this in a bug fix PR.

This is marked as resolved, but I still see this unexpectedly formatted 😕
I would expect ResolvedValue to be on the same line as the preceeding ->

As would I :( unfortunately the auto-formatter disagrees - it changes it back. I don't want to disable it just for this comment.

@kara kara added the comp: ivy label Jan 8, 2019

@ngbot ngbot bot added this to the needsTriage milestone Jan 8, 2019

@IgorMinar
Copy link
Member

IgorMinar left a comment

LGTM

@alxhub one nit: I'd prefer the feat(forms) commit message to have a more customer friendly subject that we'll end up in the changelog (e.g. list the name of main directives we are exporting)

alxhub added some commits Dec 13, 2018

feat(forms): export NumberValueAccessor & RangeValueAccessor directives
@angular/forms declares several directives and a module which are not
exported from the package via the entrypoint, either intentionally or as a
historical accident.

Ivy's locality principle necessitates that directives used in user code be
importable from the package which defines them. This requires these forms
directives to be exported.

Several directives which define ControlValueAccessors are exported:

* NumberValueAccessor
* RangeValueAccessor

A few more directives and a module are exported privately (with a ɵ prefix):

* NgNoValidate
* NgSelectMultipleOption
* InternalFormsSharedModule
feat(ivy): produce diagnostics for missing exports, incorrect entrypoint
This commit adds tracking of modules, directives, and pipes which are made
visible to consumers through NgModules exported from the package entrypoint.
ngtsc will now produce a diagnostic if such classes are not themselves
exported via the entrypoint (as this is a requirement for downstream
consumers to use them with Ivy).

To accomplish this, a graph of references is created and populated via the
ReferencesRegistry. Symbols exported via the package entrypoint are compared
against the graph to determine if any publicly visible symbols are not
properly exported. Diagnostics are produced for each one which also show the
path by which they become visible.

This commit also introduces a diagnostic (instead of a hard compiler crash)
if an entrypoint file cannot be correctly determined.
feat(ivy): reference external classes by their exported name
Previously, ngtsc would assume that a given directive/pipe being imported
from an external package was importable using the same name by which it
was declared. This isn't always true; sometimes a package will export a
directive under a different name. For example, Angular frequently prefixes
directive names with the 'ɵ' character to indicate that they're part of
the package's private API, and not for public consumption.

This commit introduces the TsReferenceResolver class which, given a
declaration to import and a module name to import it from, can determine
the exported name of the declared class within the module. This allows
ngtsc to pick the correct name by which to import the class instead of
making assumptions about how it was exported.

This resolver is used to select a correct symbol name when creating an
AbsoluteReference.

FW-517 #resolve
FW-536 #resolve

@alxhub alxhub force-pushed the alxhub:ngtsc-private-names branch from a457e09 to ea1d4e4 Jan 9, 2019

@alxhub alxhub requested a review from angular/fw-public-api as a code owner Jan 9, 2019

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Jan 9, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Jan 9, 2019

Dear karataker,

The bot still thinks I need two approvals, but @IgorMinar already approved and should count for fw-public-api, and I'm the approver for fw-compiler.

@kara kara closed this in 37b716b Jan 9, 2019

kara added a commit that referenced this pull request Jan 9, 2019

refactor(ivy): split apart the 'metadata' package in the ngtsc compil…
…er (#27743)

This refactoring moves code around between a few of the ngtsc subpackages,
with the goal of having a more logical package structure. Additional
interfaces are also introduced where they make sense.

The 'metadata' package formerly contained both the partial evaluator,
the TypeScriptReflectionHost as well as some other reflection functions,
and the Reference interface and various implementations. This package
was split into 3 parts.

The partial evaluator now has its own package 'partial_evaluator', and
exists behind an interface PartialEvaluator instead of a top-level
function. In the future this will be useful for reducing churn as the
partial evaluator becomes more complicated.

The TypeScriptReflectionHost and other miscellaneous functions have moved
into a new 'reflection' package. The former 'host' package which contained
the ReflectionHost interface and associated types was also merged into this
new 'reflection' package.

Finally, the Reference APIs were moved to the 'imports' package, which will
consolidate all import-related logic in ngtsc.

PR Close #27743

kara added a commit that referenced this pull request Jan 9, 2019

refactor(ivy): prep ngtsc and ngcc for upcoming import resolution work (
#27743)

Upcoming work to implement import resolution will change the dependencies
of some higher-level classes in ngtsc & ngcc. This necessitates changes in
how these classes are created and the lifecycle of the ts.Program in ngtsc
& ngcc.

To avoid complicating the implementation work with refactoring as a result
of the new dependencies, the refactoring is performed in this commit as a
separate prepatory step.

In ngtsc, the testing harness is modified to allow easier access to some
aspects of the ts.Program.

In ngcc, the main change is that the DecorationAnalyzer is created with the
ts.Program as a constructor parameter. This is not a lifecycle change, as
it was previously created with the ts.TypeChecker which is derived from the
ts.Program anyways. This change requires some reorganization in ngcc to
accommodate, especially in testing harnesses where DecorationAnalyzer is
created manually in a number of specs.

PR Close #27743

kara added a commit that referenced this pull request Jan 9, 2019

feat(forms): export NumberValueAccessor & RangeValueAccessor directiv…
…es (#27743)

@angular/forms declares several directives and a module which are not
exported from the package via the entrypoint, either intentionally or as a
historical accident.

Ivy's locality principle necessitates that directives used in user code be
importable from the package which defines them. This requires these forms
directives to be exported.

Several directives which define ControlValueAccessors are exported:

* NumberValueAccessor
* RangeValueAccessor

A few more directives and a module are exported privately (with a ɵ prefix):

* NgNoValidate
* NgSelectMultipleOption
* InternalFormsSharedModule

PR Close #27743

kara added a commit that referenced this pull request Jan 9, 2019

feat(ivy): produce diagnostics for missing exports, incorrect entrypo…
…int (#27743)

This commit adds tracking of modules, directives, and pipes which are made
visible to consumers through NgModules exported from the package entrypoint.
ngtsc will now produce a diagnostic if such classes are not themselves
exported via the entrypoint (as this is a requirement for downstream
consumers to use them with Ivy).

To accomplish this, a graph of references is created and populated via the
ReferencesRegistry. Symbols exported via the package entrypoint are compared
against the graph to determine if any publicly visible symbols are not
properly exported. Diagnostics are produced for each one which also show the
path by which they become visible.

This commit also introduces a diagnostic (instead of a hard compiler crash)
if an entrypoint file cannot be correctly determined.

PR Close #27743

kara added a commit that referenced this pull request Jan 9, 2019

feat(ivy): reference external classes by their exported name (#27743)
Previously, ngtsc would assume that a given directive/pipe being imported
from an external package was importable using the same name by which it
was declared. This isn't always true; sometimes a package will export a
directive under a different name. For example, Angular frequently prefixes
directive names with the 'ɵ' character to indicate that they're part of
the package's private API, and not for public consumption.

This commit introduces the TsReferenceResolver class which, given a
declaration to import and a module name to import it from, can determine
the exported name of the declared class within the module. This allows
ngtsc to pick the correct name by which to import the class instead of
making assumptions about how it was exported.

This resolver is used to select a correct symbol name when creating an
AbsoluteReference.

FW-517 #resolve
FW-536 #resolve

PR Close #27743

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

refactor(ivy): move the flat module index generator to its own package (
angular#27743)

This commit moves the FlatIndexGenerator to its own package, in preparation
to expand its capabilities and support re-exporting of private declarations
from NgModules.

PR Close angular#27743

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

refactor(ivy): split apart the 'metadata' package in the ngtsc compil…
…er (angular#27743)

This refactoring moves code around between a few of the ngtsc subpackages,
with the goal of having a more logical package structure. Additional
interfaces are also introduced where they make sense.

The 'metadata' package formerly contained both the partial evaluator,
the TypeScriptReflectionHost as well as some other reflection functions,
and the Reference interface and various implementations. This package
was split into 3 parts.

The partial evaluator now has its own package 'partial_evaluator', and
exists behind an interface PartialEvaluator instead of a top-level
function. In the future this will be useful for reducing churn as the
partial evaluator becomes more complicated.

The TypeScriptReflectionHost and other miscellaneous functions have moved
into a new 'reflection' package. The former 'host' package which contained
the ReflectionHost interface and associated types was also merged into this
new 'reflection' package.

Finally, the Reference APIs were moved to the 'imports' package, which will
consolidate all import-related logic in ngtsc.

PR Close angular#27743

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

refactor(ivy): prep ngtsc and ngcc for upcoming import resolution work (
angular#27743)

Upcoming work to implement import resolution will change the dependencies
of some higher-level classes in ngtsc & ngcc. This necessitates changes in
how these classes are created and the lifecycle of the ts.Program in ngtsc
& ngcc.

To avoid complicating the implementation work with refactoring as a result
of the new dependencies, the refactoring is performed in this commit as a
separate prepatory step.

In ngtsc, the testing harness is modified to allow easier access to some
aspects of the ts.Program.

In ngcc, the main change is that the DecorationAnalyzer is created with the
ts.Program as a constructor parameter. This is not a lifecycle change, as
it was previously created with the ts.TypeChecker which is derived from the
ts.Program anyways. This change requires some reorganization in ngcc to
accommodate, especially in testing harnesses where DecorationAnalyzer is
created manually in a number of specs.

PR Close angular#27743

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(forms): export NumberValueAccessor & RangeValueAccessor directiv…
…es (angular#27743)

@angular/forms declares several directives and a module which are not
exported from the package via the entrypoint, either intentionally or as a
historical accident.

Ivy's locality principle necessitates that directives used in user code be
importable from the package which defines them. This requires these forms
directives to be exported.

Several directives which define ControlValueAccessors are exported:

* NumberValueAccessor
* RangeValueAccessor

A few more directives and a module are exported privately (with a ɵ prefix):

* NgNoValidate
* NgSelectMultipleOption
* InternalFormsSharedModule

PR Close angular#27743

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(ivy): produce diagnostics for missing exports, incorrect entrypo…
…int (angular#27743)

This commit adds tracking of modules, directives, and pipes which are made
visible to consumers through NgModules exported from the package entrypoint.
ngtsc will now produce a diagnostic if such classes are not themselves
exported via the entrypoint (as this is a requirement for downstream
consumers to use them with Ivy).

To accomplish this, a graph of references is created and populated via the
ReferencesRegistry. Symbols exported via the package entrypoint are compared
against the graph to determine if any publicly visible symbols are not
properly exported. Diagnostics are produced for each one which also show the
path by which they become visible.

This commit also introduces a diagnostic (instead of a hard compiler crash)
if an entrypoint file cannot be correctly determined.

PR Close angular#27743

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

feat(ivy): reference external classes by their exported name (angular…
…#27743)

Previously, ngtsc would assume that a given directive/pipe being imported
from an external package was importable using the same name by which it
was declared. This isn't always true; sometimes a package will export a
directive under a different name. For example, Angular frequently prefixes
directive names with the 'ɵ' character to indicate that they're part of
the package's private API, and not for public consumption.

This commit introduces the TsReferenceResolver class which, given a
declaration to import and a module name to import it from, can determine
the exported name of the declared class within the module. This allows
ngtsc to pick the correct name by which to import the class instead of
making assumptions about how it was exported.

This resolver is used to select a correct symbol name when creating an
AbsoluteReference.

FW-517 #resolve
FW-536 #resolve

PR Close angular#27743
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.