Conversation
|
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')); |
There was a problem hiding this comment.
Why de-abstract this?
fa37776 to
a2e7dd7
Compare
|
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'; |
| // (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)); |
There was a problem hiding this comment.
Is it guaranteed that files does not contains any .js files?
| 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'; |
There was a problem hiding this comment.
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; | ||
| } |
There was a problem hiding this comment.
Now that these classes are public API, it would be better to have more descriptive param names.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Order doesn't match all other places 😁 😇
|
|
||
| // Neither is the module which declares it - meaning the directive is not visible here. | ||
| @NgModule({declarations: [Dir], exports: [Dir]}) | ||
| class DirModule {} |
There was a problem hiding this comment.
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.
| export class ReferenceGraph<T = ts.Declaration> { | ||
| private references = new Map<T, Set<T>>(); | ||
|
|
||
| constructor() {} |
| return null; | ||
| } else { | ||
| // Look through the outgoing edges of `source`. | ||
| // TODO(alxhub): use proper iteration when build.sh is removed. (#27762) |
There was a problem hiding this comment.
Could you at least use for ... of Array.from(set) (until build.sh is removed)?
There was a problem hiding this comment.
This would allocate, which I'm trying to avoid.
There was a problem hiding this comment.
🎉🍾🎆
I'll go through and convert back to beautiful native iteration.
There was a problem hiding this comment.
Noooooo!!
The legacy build on CircleCI is basically build.sh reincarnated. It has the same issue 😭
| Reference<ts.Declaration>; | ||
| } | ||
|
|
||
| export class NpmReferenceResolver implements ReferenceResolver { |
There was a problem hiding this comment.
Possibly a n00b question, but...why Npm?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
This comment is irrelevant here.
AndrewKushnir
left a comment
There was a problem hiding this comment.
Looks great, thanks @alxhub.
a2e7dd7 to
44d4413
Compare
|
@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 Thus, I think my refactor actually makes the API better, and the other analyzer should follow suit. |
|
You can preview 44d4413 at https://pr27743-44d4413.ngbuilds.io/. |
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.
44d4413 to
da6414b
Compare
|
You can preview da6414b at https://pr27743-da6414b.ngbuilds.io/. |
1ed0cf4 to
a457e09
Compare
|
You can preview 1ed0cf4 at https://pr27743-1ed0cf4.ngbuilds.io/. |
|
You can preview a457e09 at https://pr27743-a457e09.ngbuilds.io/. |
gkalpak
left a comment
There was a problem hiding this comment.
LGTM (with a couple of minor comments)
| * 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> { |
There was a problem hiding this comment.
Then why doesn't it have expressable = false? 😕
| * | ||
| * This is a reified type to allow the circular reference of `ResolvedValue` -> `ResolvedValueArray` | ||
| * -> | ||
| * `ResolvedValue`. |
There was a problem hiding this comment.
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.)
You're correct, and this should be unified, but it's outside the scope of this PR for now.
It should - I'll fix this in a bug fix PR.
As would I :( unfortunately the auto-formatter disagrees - it changes it back. I don't want to disable it just for this comment. |
@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
a457e09 to
ea1d4e4
Compare
|
You can preview ea1d4e4 at https://pr27743-ea1d4e4.ngbuilds.io/. |
|
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. |
…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
#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
…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
…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
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
|
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. |
No description provided.