Skip to content

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

Closed
alxhub wants to merge 6 commits into
angular:masterfrom
alxhub:ngtsc-private-names
Closed

alxhub wants to merge 6 commits into
angular:masterfrom
alxhub:ngtsc-private-names

Conversation

@alxhub

@alxhub alxhub commented Dec 19, 2018

Copy link
Copy Markdown
Member

No description provided.

@alxhub alxhub added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 19, 2018
@mary-poppins

Copy link
Copy Markdown

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

// (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'));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why de-abstract this?

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.

Fixed :)

@alxhub
alxhub force-pushed the ngtsc-private-names branch from fa37776 to a2e7dd7 Compare December 19, 2018 23:57
@mary-poppins

Copy link
Copy Markdown

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';

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.

Unused import?

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.

Fixed!

// (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));

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.

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

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.

No, it's not. Fixed.

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';

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.

Join with the above export?

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.

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;
}

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.

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

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.

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.

export interface BundleProgram {
program: ts.Program;
host: ts.CompilerHost;
options: ts.CompilerOptions;

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.

Order doesn't match all other places 😁 😇

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.

Switched!


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

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 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.

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.

Added!

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

constructor() {}

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.

Redundant empty constructor.

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.

Removed.

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

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 at least use for ... of Array.from(set) (until build.sh is removed)?

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.

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

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.

build.sh has been removed in #27937 fwiw 😉

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.

🎉🍾🎆

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

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.

Noooooo!!

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

Reference<ts.Declaration>;
}

export class NpmReferenceResolver implements ReferenceResolver {

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.

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

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.

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.

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 comment is irrelevant here.

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.

Fixed!

Comment thread packages/compiler-cli/src/ngcc/src/packages/transformer.ts Outdated

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, thanks @alxhub.

@alxhub
alxhub force-pushed the ngtsc-private-names branch from a2e7dd7 to 44d4413 Compare January 7, 2019 23:55
@alxhub
alxhub requested review from a team January 7, 2019 23:55
@alxhub

alxhub commented Jan 8, 2019

Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown

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

alxhub added 3 commits January 8, 2019 09:41
This commit moves the FlatIndexGenerator to its own package, in preparation
to expand its capabilities and support re-exporting of private declarations
from NgModules.
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.
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.
@alxhub
alxhub force-pushed the ngtsc-private-names branch from 44d4413 to da6414b Compare January 8, 2019 18:20
@alxhub
alxhub requested a review from kara January 8, 2019 18:24
@mary-poppins

Copy link
Copy Markdown

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

@alxhub
alxhub force-pushed the ngtsc-private-names branch 2 times, most recently from 1ed0cf4 to a457e09 Compare January 8, 2019 18:32
@mary-poppins

Copy link
Copy Markdown

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

@mary-poppins

Copy link
Copy Markdown

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

@kara kara left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@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.

LGTM (with a couple of minor comments)

Comment thread 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> {

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.

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

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

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 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 mentioned this pull request Jan 8, 2019
@alxhub

alxhub commented Jan 8, 2019

Copy link
Copy Markdown
Member Author

@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 IgorMinar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 3 commits January 8, 2019 16:05
@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
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.
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 ngtsc-private-names branch from a457e09 to ea1d4e4 Compare January 9, 2019 00:07
@alxhub
alxhub requested a review from a team January 9, 2019 00:07
@mary-poppins

Copy link
Copy Markdown

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

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 9, 2019
@alxhub

alxhub commented Jan 9, 2019

Copy link
Copy Markdown
Member Author

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 pushed a commit that referenced this pull request Jan 9, 2019
…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 pushed a commit that referenced this pull request Jan 9, 2019
#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 pushed a commit that referenced this pull request Jan 9, 2019
…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 pushed a commit that referenced this pull request Jan 9, 2019
…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 pushed a commit that referenced this pull request Jan 9, 2019
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
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants